-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RestoreDB new Task #98
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! It looks great! I haven't tested it yet, but looks good to me. I left some small comments.
|
||
c.run(f"{DOCKER_COMPOSE_COMMAND} 'start database'", pty=True) | ||
|
||
c.run(f"docker cp {file} community_database_1:/tmp/dump.sql") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you need to use the same trick we use in https://github.com/readthedocs/common/pull/98/files#diff-302a2219ef42d627f0daf3e4752a1838c41e64b43d7912f149f10cd04f6c30c0R109 because community_
is only for our community code, but for corporate code (readthedocs.com) that should be commercial_
instead.
So, you need to grab that value from the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vitorsv1 would you like to update this PR with this change requested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be possible to use {DOCKER_COMPOSE_PROJECT_NAME}_database_1
in this string following #145
@vitorsv1 Hi! Do you have the time to finish this work? 😅 |
Unfortunately I don't have time to review anymore @humitos :( I'm working in two jobs right now |
Don't worry. Thanks for your work! I'll will try to update it and merge 👍 |
@humitos I see that the issue is not resolved yet. Are you still planning on getting this PR merged? If so, I can start working on it. This will be a good first issue for me given that i am new to Read the Docs repo. I did understand the issue partially and trying to understand 'the trick' that you pointed out in the comments (but no luck). The link you gave points back to file differences and nothing in specific. Perhaps, as @benjaoming mentioned, we can use the DOCKER_COMPOSE_PROJECT_NAME variable now that it is the part of Read The Docs Project. |
References
ISSUE - readthedocs/readthedocs.org#7761
PR - readthedocs/readthedocs.org#8708