-
Notifications
You must be signed in to change notification settings - Fork 51
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
Ghsync integration #54
Conversation
environment: | ||
GHSYNC_ORG: ${GITHUB_ORGANIZATION:-} | ||
GHSYNC_TOKEN: ${GITHUB_TOKEN:-} | ||
GHSYNC_POSTGRES_DB: metadata |
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'd suggest the same than in src-d/ghsync#25 (comment)
I'd let the user choose it, and use metadata
as a default value
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.
we don't allow to choose names of maindb or gitbase from the cli. Why should we allow to choose a name for metadata?
And of course, the user still can change all the names in docker-compose.yml manually.
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.
imo using vars also helps to understand the relations between different parts, but not a requirement to approve this PR, which LGTM
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.
makes sense to use variables to avoid repeated hard-coded values in different containers! 👍
shouldn't be done in the PR though. I created an issue: #62
docker-compose.yml
Outdated
# wait for db to be created | ||
# we need to use something like https://github.com/vishnubob/wait-for-it | ||
# or implement wait in ghsync itself | ||
command: ['-c', 'sleep 10s && ghsync migrate && ghsync fastsync'] |
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.
command: ['-c', 'sleep 10s && ghsync migrate && ghsync fastsync'] | |
command: ['-c', 'sleep 10s && ghsync migrate && ghsync shallow'] |
Only docker-compose.yml needs to be changed Signed-off-by: Maxim Sukharev <[email protected]>
Fix: #52
Based on #51 only the last commit is new
Only docker-compose.yml needs to be changed