-
Notifications
You must be signed in to change notification settings - Fork 12
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
add starship requirements in dockerfile, add docker gh action #61
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.
LGTM!
Just some questions to better understand the changes
demo/Dockerfile
Outdated
@@ -31,9 +30,15 @@ FROM alpine:3.16 | |||
|
|||
COPY --from=go-builder /code/build/meshd /usr/bin/meshd | |||
|
|||
# Set up dependencies used for Starship | |||
ENV PACKAGES curl make bash jq sed |
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.
Why do you use an ENV for this and not just add it to L37?
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.
Can do. will change it.
branches: | ||
- main | ||
tags: | ||
- "v*.*.*" |
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 checking, does this covers RC tags like v0.1.2-rc.3
as well?
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 just took it from: https://github.com/osmosis-labs/mesh-security-sdk/blob/main/.github/workflows/release.yml#L6
I would like to have something like:
https://github.com/cosmos/cosmos-sdk/blob/main/.github/workflows/docker.yml#L11...L13
But did not want to deviate from the repo practices
|
||
permissions: | ||
contents: read | ||
packages: write |
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 need this to push to the docker registry, right?
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.
yup
uses: docker/metadata-action@v4 | ||
with: | ||
images: ${{ env.DOCKER_REGISTRY }}/${{ env.DOCKER_IMAGE }} | ||
tags: | |
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.
Do you use the tags for anything?
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.
for versioning. As more versions of the mesh-security-sdk comes, we should be able to use the tagged version
|
||
- name: Run go vendor | ||
run: | | ||
cd demo/ && go mod vendor |
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.
Agh, you need to vendor here, too. This is very inconvenient
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.
Yes, useally i would just have this part of the dockerfile itself, but the current system seemed to take pre built vendor dir
Overview
Add starship requirements into the dockerfile
Add gh-action to push docker images to ghcr.io at:
Closes #53