-
Notifications
You must be signed in to change notification settings - Fork 22
feat: docker support #238
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
feat: docker support #238
Conversation
README.md
Outdated
|
||
#### Run with Environment Variables | ||
|
||
You need to provide either a MongoDB connection string OR Atlas API credentials: |
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 need the connection string anymore, though we should probably preface that with a warning that if you don't, you'll be exposing the connection string to the LLM.
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.
updated readme
This reverts commit 6c714b3.
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.
Just nits around the GHA workflows from me. I've turned off the auto-merge so you get a chance to look at the comments and address those that make sense to you, but no objections to merging it once we update the workflow permissions.
steps: | ||
- uses: GitHubSecurityLab/actions-permissions/monitor@v1 | ||
with: | ||
config: ${{ vars.PERMISSIONS_CONFIG }} |
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.
What is this? Doesn't seem like it's created.
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.
According to https://github.com/GitHubSecurityLab/actions-permissions/tree/v1/monitor, it is advisable to keep a variable even if it does not exist yet.
.github/workflows/docker.yaml
Outdated
uses: docker/login-action@343f7c4344506bcbf9b4de18042ae17996df046d | ||
with: | ||
username: "${{ secrets.DOCKERHUB_USERNAME }}" | ||
password: "${{ secrets.DOCKERHUB_SECRET }}" |
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.
Not blocking, but should we rename this secret to DOCKERHUB_PASSWORD
?
.github/workflows/docker.yaml
Outdated
push: | ||
runs-on: ubuntu-latest | ||
env: | ||
IMAGE_REPOSITORY: docker.io/mongodb/mongodb-mcp-server |
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.
Should we move this to a repo-level variable?
.github/workflows/docker.yaml
Outdated
with: | ||
username: "${{ secrets.DOCKERHUB_USERNAME }}" | ||
password: "${{ secrets.DOCKERHUB_SECRET }}" | ||
- name: Set properties |
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.
[nit] This name is a bit vague, forcing readers to actually read the script being executed - would it make sense to replace with "Set date and version"?
push: true | ||
build-args: | | ||
VERSION=${{ steps.set-properties.outputs.VERSION }} | ||
- uses: mongodb-js/devtools-shared/actions/setup-bot-token@main |
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 typically use the bot in cases where the actions token wouldn't work - e.g. if we want to open a PR, but also run the associated workflows (which wouldn't happen with the GHA token). In this case, unless we want to run follow-up workflows on the newly created issue, we should be able to just use the GHA token. Alternatively, if we need the bot token, we can remove the issues: write
permissions as those don't really affect it.
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.
This was intentional, as we intend to run GitHub actions on GitHub Issues (Jira integration).
If an issue is created by Github Actions user, it won't run further GitHub Actions.
fixes #236