ci: push devcontainer on workflow dispatch #2485
Conversation
WalkthroughOhayo, sensei! The changes in this pull request enhance the GitHub Actions workflow for managing Docker images. The configuration now accommodates pushing images on manual triggers alongside existing event types. The Changes
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/devcontainer.yml (2)
Line range hint
30-55: Ohayo, sensei! Excellent refinement of Docker tag logicThe changes to the
DOCKER_TAGsetting logic are well-implemented and improve the consistency across different event types. The use of a short SHA for push events and allowing custom tags for manual triggers are particularly good practices.One small suggestion to improve readability:
Consider using a single step to set both the environment variable and the output for all event types. This could reduce repetition and make the workflow more maintainable. Here's an example:
- name: Set Docker tag and output run: | if [[ "${{ github.event_name }}" == "release" ]]; then DOCKER_TAG="${{ github.event.release.tag_name }}" elif [[ "${{ github.event_name }}" == "push" ]]; then DOCKER_TAG=$(echo "${{ github.sha }}" | cut -c 1-7) else DOCKER_TAG="${{ inputs.docker_tag }}" fi echo "DOCKER_TAG=$DOCKER_TAG" >> $GITHUB_ENV echo "tag_name=$DOCKER_TAG" >> $GITHUB_OUTPUTThis consolidates the logic into a single step, making it easier to maintain and understand.
🧰 Tools
🪛 actionlint
65-65: the runner of "docker/build-push-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
Line range hint
76-105: Ohayo, sensei! Well-structured updates to the PR creation processThe changes to the
propose-update-prjob are well-thought-out. Moving the condition to the job level is a great improvement for clarity and efficiency. The logic for updating the relevant files is sound and ensures consistency across the project.One small suggestion for consistency:
Consider using the
${{ env.DOCKER_TAG }}instead of${{ needs.build-and-push.outputs.tag_name }}in the sed commands. This would make the tag reference consistent with thebuild-and-pushjob. Here's an example for the devcontainer.json update:- name: Update devcontainer.json run: sed -i "s|ghcr.io/dojoengine/dojo-dev:[a-zA-Z0-9._-]*|ghcr.io/dojoengine/dojo-dev:${{ env.DOCKER_TAG }}|" .devcontainer/devcontainer.jsonMake sure to apply this change to the GitHub action devcontainers update step as well.
🧰 Tools
🪛 actionlint
65-65: the runner of "docker/build-push-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/devcontainer.yml (1 hunks)
🔇 Additional comments (1)
.github/workflows/devcontainer.yml (1)
67-67: Ohayo, sensei! LGTM: Enhanced flexibility for Docker image pushingThe addition of the
workflow_dispatchevent to the push condition is a great improvement. It allows for manual triggering of the Docker image build and push process, which can be very useful for testing or ad-hoc deployments.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- .devcontainer/Dockerfile (1 hunks)
- .github/workflows/devcontainer.yml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/devcontainer.yml
| elif [ "$BUILD_TYPE" = "workflow_dispatch" ]; then \ | ||
| echo "Installing Dojo at commit $DOJO_VERSION"; \ | ||
| dojoup -c $DOJO_VERSION; \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Avoid code duplication by consolidating conditions
The blocks for push and workflow_dispatch are performing the same actions. Consider combining them to simplify the script and reduce redundancy.
Apply this diff to refactor the code:
RUN if [ "$BUILD_TYPE" = "release" ]; then \
echo "Installing Dojo version $DOJO_VERSION"; \
dojoup -v $DOJO_VERSION; \
-elif [ "$BUILD_TYPE" = "push" ]; then \
+elif [ "$BUILD_TYPE" = "push" ] || [ "$BUILD_TYPE" = "workflow_dispatch" ]; then \
echo "Installing Dojo at commit $DOJO_VERSION"; \
dojoup -c $DOJO_VERSION; \
-elif [ "$BUILD_TYPE" = "workflow_dispatch" ]; then \
- echo "Installing Dojo at commit $DOJO_VERSION"; \
- dojoup -c $DOJO_VERSION; \
else \
echo "Installing latest stable version of Dojo"; \
dojoup; \
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif [ "$BUILD_TYPE" = "workflow_dispatch" ]; then \ | |
| echo "Installing Dojo at commit $DOJO_VERSION"; \ | |
| dojoup -c $DOJO_VERSION; \ | |
| elif [ "$BUILD_TYPE" = "push" ] || [ "$BUILD_TYPE" = "workflow_dispatch" ]; then \ | |
| echo "Installing Dojo at commit $DOJO_VERSION"; \ | |
| dojoup -c $DOJO_VERSION; \ |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
.github/workflows/devcontainer.yml (1)
46-46: Ohayo, sensei! Nice addition of DOJO_VERSION!The addition of the
DOJO_VERSIONenvironment variable is a good improvement for consistency in version tracking. It aligns well with the existingDOCKER_TAGlogic.Consider using double quotes for the environment variable assignment to prevent potential issues with special characters:
- echo "DOJO_VERSION=${{ github.event.release.tag_name }}" >> $GITHUB_ENV + echo "DOJO_VERSION=\"${{ github.event.release.tag_name }}\"" >> $GITHUB_ENV
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/devcontainer.yml (2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/devcontainer.yml
51-51: shellcheck reported issue in this script: SC2086:info:2:33: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:3:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:3:47: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/devcontainer.yml (2)
Line range hint
1-15: Ohayo, sensei! LGTM on the workflow trigger update!The addition of the
workflow_dispatchevent with adocker_taginput is a great improvement. It allows for manual triggering of the workflow and provides flexibility in naming the Docker image. This change enhances the usability of the CI process.🧰 Tools
🪛 actionlint
44-44: shellcheck reported issue in this script: SC2086:info:1:59: Double quote to prevent globbing and word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2086:info:2:61: Double quote to prevent globbing and word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2086:info:3:32: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:2:33: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:3:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:3:47: Double quote to prevent globbing and word splitting
(shellcheck)
71-77: Ohayo, sensei! Excellent updates to the build and push step!The changes in this step are well-thought-out and improve the workflow significantly:
The
pushcondition now includes theworkflow_dispatchevent, allowing manual triggers to push Docker images. This adds flexibility to the CI process.The addition of
DOJO_VERSIONas a build argument ensures that the version information is available during the build process, which can be crucial for versioning within the container.These changes align well with the previous updates and enhance the overall functionality of the workflow.
.github/workflows/devcontainer.yml
Outdated
| echo "tag_name=$DOCKER_TAG" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Set Docker tag for push event | ||
| if: github.event_name == 'push' | ||
| run: | | ||
| SHORT_SHA=$(echo "${{ github.sha }}" | cut -c 1-7) | ||
| echo "DOCKER_TAG=$SHORT_SHA" >> $GITHUB_ENV | ||
| echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Ohayo, sensei! Good addition, but let's align it with the release event.
The addition of the DOJO_VERSION is great for consistency. However, there's a small inconsistency in how it's being set.
In the release event, DOJO_VERSION is set to $GITHUB_ENV, but here it's set to $GITHUB_OUTPUT. Let's make it consistent:
- echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_OUTPUT
+ echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_ENVAlso, consider using double quotes as suggested in the previous comment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_OUTPUT | |
| echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_ENV |
.github/workflows/devcontainer.yml
Outdated
| SHORT_SHA=$(echo "${{ github.sha }}" | cut -c 1-7) | ||
| echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Ohayo, sensei! Good additions, but let's refine them a bit.
The addition of SHORT_SHA and DOJO_VERSION is good for consistency. However, there are a couple of points to consider:
- The
DOJO_VERSIONis set to$GITHUB_OUTPUTinstead of$GITHUB_ENV, similar to the push event. Let's make it consistent:
- echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_OUTPUT
+ echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_ENV- Consider using the user-provided
docker_tagforDOJO_VERSIONinstead ofSHORT_SHA, to align with theDOCKER_TAGlogic:
- echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_ENV
+ echo "DOJO_VERSION=${{ inputs.docker_tag }}" >> $GITHUB_ENVAlso, remember to use double quotes for the environment variable assignments.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/devcontainer.yml (2)
54-54: Ohayo, sensei! Good addition, but let's make it even better.The
DOJO_VERSIONis correctly set to the short SHA for push events, maintaining consistency with theDOCKER_TAG. However, we can improve it slightly:Consider using double quotes to prevent potential issues with special characters:
- echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_ENV + echo "DOJO_VERSION=\"$SHORT_SHA\"" >> $GITHUB_ENV
77-77: Ohayo, sensei! Great addition of DOJO_VERSION as a build arg.Passing
DOJO_VERSIONas a build argument is an excellent way to make the version information available during the Docker build process. This can be useful for versioning within the container or for other build-time decisions.For consistency with other build args, consider using quotes:
- DOJO_VERSION=${{ env.DOJO_VERSION }} + DOJO_VERSION="${{ env.DOJO_VERSION }}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/devcontainer.yml (2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/devcontainer.yml
51-51: shellcheck reported issue in this script: SC2086:info:2:33: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:3:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:3:47: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/devcontainer.yml (3)
46-46: Ohayo, sensei! Nice addition of DOJO_VERSION for releases.The
DOJO_VERSIONis correctly set to the release tag name for the release event. This ensures consistency between the release version and the Docker image tag.
71-71: Ohayo, sensei! Excellent update to the push condition.The addition of
github.event_name == 'workflow_dispatch'to the push condition is spot on. This change allows manual triggering of image pushes, which perfectly aligns with the PR objective of pushing the devcontainer on workflow dispatch.
Line range hint
1-114: Ohayo, sensei! Overall, these changes are dojo-tastic!The modifications to the workflow successfully implement the PR objective of pushing the devcontainer on workflow dispatch. The changes maintain consistency in version labeling across different event types and enhance the flexibility of the CI process.
A few minor suggestions have been made to improve code robustness and consistency. Once these are addressed, the changes will be even more solid.
Great work on enhancing the CI workflow, sensei!
🧰 Tools
🪛 actionlint
44-44: shellcheck reported issue in this script: SC2086:info:1:59: Double quote to prevent globbing and word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2086:info:2:61: Double quote to prevent globbing and word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2086:info:3:32: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:2:33: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:3:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:3:47: Double quote to prevent globbing and word splitting
(shellcheck)
| SHORT_SHA=$(echo "${{ github.sha }}" | cut -c 1-7) | ||
| echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_ENV |
There was a problem hiding this comment.
Ohayo, sensei! Let's clarify the version setting for workflow_dispatch.
The addition of SHORT_SHA and DOJO_VERSION for the workflow_dispatch event is good, but there's a potential inconsistency:
DOJO_VERSIONis set to the short SHA, whileDOCKER_TAGuses the user input.- We might want to use double quotes to prevent issues with special characters.
Consider the following changes:
- SHORT_SHA=$(echo "${{ github.sha }}" | cut -c 1-7)
- echo "DOJO_VERSION=$SHORT_SHA" >> $GITHUB_ENV
+ SHORT_SHA=$(echo "${{ github.sha }}" | cut -c 1-7)
+ echo "DOJO_VERSION=\"${{ inputs.docker_tag }}\"" >> $GITHUB_ENVThis aligns DOJO_VERSION with DOCKER_TAG for consistency. However, if there's a specific reason for using the short SHA instead of the user input for DOJO_VERSION, please clarify.
Committable suggestion was skipped due to low confidence.
|
Use preview instead. Thanks @steebchen. <3 |
Summary by CodeRabbit
hurl.