Added registry, image-name, docker-context, and docker-file parameters to docker workflow - #4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds configurable parameters (registry, image-name, docker-context, and docker-file) to the Docker workflow, replacing hardcoded environment variables with flexible inputs. This allows other projects to customize their Docker build setup while maintaining backward compatibility through sensible defaults.
Key Changes
- Removed hardcoded
REGISTRYandIMAGE_NAMEenvironment variables from the workflow - Added four new optional input parameters with defaults
- Updated documentation to describe the new parameters
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/docker.yml |
Replaced environment variables with configurable workflow inputs for registry, image name, Docker context, and Dockerfile path |
wiki/Getting-Started.md |
Added documentation for the four new workflow parameters in the arguments table |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mowi12 Feel free to test this out for your other project. If there are any other parameters required, I can add them. |
Will do! I will tell you, if there is anything missing or if we can continue merging this PR. |
1dd76a2 to
7db569d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7db569d to
5503059
Compare
5503059 to
81d4191
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Login to Container Registry | ||
| if: steps.check-push.outputs.should-push == 'true' | ||
| uses: docker/login-action@v3 | ||
| with: | ||
| registry: ${{ env.REGISTRY }} | ||
| registry: ${{ inputs.registry }} | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
The login step uses hardcoded github.actor as username and secrets.GITHUB_TOKEN as password, which only works for GitHub Container Registry (ghcr.io). Since the workflow now accepts a configurable registry parameter, users might specify other registries like Docker Hub or AWS ECR, where these credentials would be invalid. Consider adding input parameters for registry username and password/token, or document that this workflow only supports ghcr.io despite the registry parameter.
81d4191 to
ddf1b59
Compare
ddf1b59 to
162606b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type: string | ||
| default: . | ||
| docker-file: | ||
| description: The Dockerfile to build. |
There was a problem hiding this comment.
The docker-file parameter description should clarify that the path should be relative to the docker-context parameter, not the repository root. This is important for users to understand when providing custom values. For example, if docker-context is set to ./app and the Dockerfile is at ./app/build/Dockerfile, then docker-file should be set to build/Dockerfile, not ./app/build/Dockerfile.
| description: The Dockerfile to build. | |
| description: The Dockerfile to build, specified as a path relative to 'docker-context'. For example, if 'docker-context' is './app' and the Dockerfile is './app/build/Dockerfile', set this to 'build/Dockerfile'. |
| - name: Determine image name | ||
| id: image-name | ||
| run: | | ||
| echo "image-name=${{ inputs.image-name == '' && github.repository || inputs.image-name }}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
The conditional expression uses an empty string check (inputs.image-name == '') which works but is not the idiomatic way to check for empty values in GitHub Actions. Consider using inputs.image-name || github.repository which is more concise and follows GitHub Actions best practices for providing fallback values.
| echo "image-name=${{ inputs.image-name == '' && github.repository || inputs.image-name }}" >> $GITHUB_OUTPUT | |
| echo "image-name=${{ inputs.image-name || github.repository }}" >> $GITHUB_OUTPUT |
| required: true | ||
| type: string | ||
| registry: | ||
| description: The registry to which the image should be pushed. |
There was a problem hiding this comment.
The description says "The registry to which the image should be pushed" but this doesn't clarify that the registry must be compatible with GitHub's authentication mechanism (github.actor and GITHUB_TOKEN). Since the workflow currently only supports GitHub Container Registry authentication, this limitation should be documented in the description.
| description: The registry to which the image should be pushed. | |
| description: The container registry to which the image should be pushed. Must support authentication using github.actor and GITHUB_TOKEN (this workflow currently supports GitHub Container Registry, ghcr.io). |
162606b to
8acae27
Compare
|
@Slartibartfass2 let's finish this. I know we said we can keep it open until we have more features or actually use them but this is not the recommended workflow for CICD, remember ;) I would rather create an additional issue+PR if we need something specific. This also let's us start more cleanly into the new semester. |
8acae27 to
159e9fa
Compare
Closes #3
What I have made
This adds parameters to the docker workflow so that it can be used by others with different setups.
Checklist
Either tick or cross out the items that do not apply (using ~~example text~~) and give a reason why the item does not apply.
Author
Reviewer