ci(github): Refactor clp-core-build action into an action that can run an arbitrary command on a container image. - #748
Conversation
WalkthroughThis pull request updates the GitHub workflow configuration by switching from the legacy Changes
Sequence Diagram(s)sequenceDiagram
participant W as Workflow Job
participant A as run-on-image Action
participant D as Docker Container
participant R as Repository
W->>A: Start action with inputs (image_name, use_published_image, run_command)
alt use_published_image is false
A->>A: Download image artifact
A->>D: Load image into Docker from tar file
else use_published_image is true
A->>A: Generate qualified image name
end
A->>A: Initialise dependencies
A->>D: Mount repository (R)
A->>D: Execute run_command inside container
D-->>W: Return execution result
Suggested reviewers
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/clp-core-build.yaml (1)
225-241: Artifact Upload Step for Ubuntu Jammy Binaries
This step conditionally copies and archives the binaries for artifact uploading. The tar command and directory handling appear correct; however, consider adding error handling in case the tar operation fails. This could help with debugging any potential issues during the upload process..github/actions/clp-core-image-run/action.yaml (1)
45-52: Docker Run Execution with Custom Command
The final step executes a docker run command that appends the custom bash script (${{inputs.run_command}}). While this simplification is welcomed, please verify that the command string is properly quoted to avoid shell interpretation issues. It might also be beneficial to log the executed command for debugging purposes in future iterations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/clp-core-image-run/action.yaml(3 hunks).github/workflows/clp-core-build.yaml(5 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/clp-core-build.yaml
214-214: property "ubuntu_focal_image_changed" is not defined in object type {centos_stream_9_image_changed: string; clp_changed: string; ubuntu_jammy_image_changed: string}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (5)
.github/workflows/clp-core-build.yaml (3)
7-7: New Action File Reference Added (Pull Request Paths)
A new file reference for the action (.github/actions/clp-core-image-run/action.yaml) has been added to the pull_request paths. Please ensure this new reference is maintained consistently across all triggers.
18-18: New Action File Reference Added (Push Paths)
The push paths now include the new action file (.github/actions/clp-core-image-run/action.yaml). Verify that the file path is correct and that no unintended files are being watched.
166-181: Refactored Action for CentOS-Stream-9 Binaries
The job now leverages the newclp-core-image-runaction along with a customrun_commandparameter. The command appears to combine tasks correctly. Please double-check that the updated command syntax meets your intended behaviour and that the environment variables are properly resolved during runtime..github/actions/clp-core-image-run/action.yaml (2)
1-1: Renamed Action
The action’s name has been updated to "clp-core-image-run". This is in line with the new functionality for running arbitrary bash scripts.
11-13: New Input Parameter: run_command
The addition of therun_commandinput parameter enables the action to execute a custom Bash script. Ensure that this input is validated in usage and that any complex commands are appropriately escaped when passed into the docker run environment.
| - uses: "./.github/actions/clp-core-image-run" | ||
| with: | ||
| image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" | ||
| use_shared_libs: "${{matrix.use_shared_libs}}" | ||
| use_published_image: >- | ||
| ${{needs.filter-relevant-changes.outputs.ubuntu_jammy_image_changed == 'false' | ||
| ${{needs.filter-relevant-changes.outputs.ubuntu_focal_image_changed == 'false' | ||
| || (github.event_name != 'pull_request' && github.ref == 'refs/heads/main')}} | ||
| upload_binaries: "${{matrix.upload_binaries}}" | ||
| binaries_artifact_name: "${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}}" | ||
| run_command: >- | ||
| task deps:core && | ||
| python3 /mnt/clp/components/core/tools/scripts/utils/build-and-run-unit-tests.py | ||
| ${{matrix.use_shared_libs == 'true' && '--use-shared-libs' || ''}} | ||
| --source-dir /mnt/clp/components/core | ||
| --build-dir /mnt/clp/components/core/build | ||
| --num-jobs $(getconf _NPROCESSORS_ONLN) | ||
|
|
There was a problem hiding this comment.
Incorrect Output Variable Reference in Ubuntu Jammy Job
The use_published_image input is currently comparing against needs.filter-relevant-changes.outputs.ubuntu_focal_image_changed, which does not exist. The correct output variable appears to be ubuntu_jammy_image_changed. Please update the reference as shown below:
- ${{needs.filter-relevant-changes.outputs.ubuntu_focal_image_changed == 'false'
- || (github.event_name != 'pull_request' && github.ref == 'refs/heads/main')}}
+ ${{needs.filter-relevant-changes.outputs.ubuntu_jammy_image_changed == 'false'
+ || (github.event_name != 'pull_request' && github.ref == 'refs/heads/main')}}📝 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.
| - uses: "./.github/actions/clp-core-image-run" | |
| with: | |
| image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" | |
| use_shared_libs: "${{matrix.use_shared_libs}}" | |
| use_published_image: >- | |
| ${{needs.filter-relevant-changes.outputs.ubuntu_jammy_image_changed == 'false' | |
| ${{needs.filter-relevant-changes.outputs.ubuntu_focal_image_changed == 'false' | |
| || (github.event_name != 'pull_request' && github.ref == 'refs/heads/main')}} | |
| upload_binaries: "${{matrix.upload_binaries}}" | |
| binaries_artifact_name: "${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}}" | |
| run_command: >- | |
| task deps:core && | |
| python3 /mnt/clp/components/core/tools/scripts/utils/build-and-run-unit-tests.py | |
| ${{matrix.use_shared_libs == 'true' && '--use-shared-libs' || ''}} | |
| --source-dir /mnt/clp/components/core | |
| --build-dir /mnt/clp/components/core/build | |
| --num-jobs $(getconf _NPROCESSORS_ONLN) | |
| - uses: "./.github/actions/clp-core-image-run" | |
| with: | |
| image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" | |
| use_published_image: >- | |
| ${{needs.filter-relevant-changes.outputs.ubuntu_jammy_image_changed == 'false' | |
| || (github.event_name != 'pull_request' && github.ref == 'refs/heads/main')}} | |
| run_command: >- | |
| task deps:core && | |
| python3 /mnt/clp/components/core/tools/scripts/utils/build-and-run-unit-tests.py | |
| ${{matrix.use_shared_libs == 'true' && '--use-shared-libs' || ''}} | |
| --source-dir /mnt/clp/components/core | |
| --build-dir /mnt/clp/components/core/build | |
| --num-jobs $(getconf _NPROCESSORS_ONLN) |
🧰 Tools
🪛 actionlint (1.7.4)
214-214: property "ubuntu_focal_image_changed" is not defined in object type {centos_stream_9_image_changed: string; clp_changed: string; ubuntu_jammy_image_changed: string}
(expression)
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
ci(github): Refactor clp-core-build action into an action that can run an arbitrary command on a container image.
| @@ -1,4 +1,4 @@ | |||
| name: "clp-core-build" | |||
| name: "clp-core-image-run" | |||
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
davidlion
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/actions/clp-core-image-run/action.yaml (1)
11-13: Introduction of therun_commandInput ParameterAdding the
run_commandinput is a valuable enhancement that provides flexibility for executing user-defined commands. Ensure that users receive clear documentation or examples to prevent ambiguity in its usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/clp-core-image-run/action.yaml(3 hunks).github/workflows/clp-core-build.yaml(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (8)
.github/workflows/clp-core-build.yaml (5)
7-7: New Action File Path Added in Pull Request TriggerThe addition of the new path for
.github/actions/clp-core-image-run/action.yamlin the pull_request trigger is appropriate. It ensures that changes to the new action are correctly monitored.
18-18: Updated Push Trigger to Include New Action FileIncluding
.github/actions/clp-core-image-run/action.yamlin the push trigger paths is a good update. This change ensures that pushes affecting the new action file correctly trigger the workflow.
166-180: Refactor to Use the New Image-Run Action in CentOS JobThe job now leverages the new
clp-core-image-runaction instead of the previous build action. The newrun_commandparameter is correctly constructed to chain the tasks (runningtask deps:corefollowed by the unit test script) and conditionally include the--use-shared-libsflag.
Please ensure that the inline conditional and the command concatenation maintain the correct spacing and quoting when passed tobash -c.
211-224: Consistent Application of New Action in Ubuntu JobThe Ubuntu job step has been updated to utilise the new
clp-core-image-runaction with a similarly structuredrun_command. Confirm that the dynamic parameter (i.e. the conditional shared library flag) is handled consistently with the CentOS job.
225-232: Well-Defined Conditional for Binary UploadThe conditional step for handling binary artifacts (triggered when
matrix.upload_binaries == 'true') is clear and aligns with expected practices. Please verify that the output directory and tarball creation do not conflict with earlier steps in the pipeline..github/actions/clp-core-image-run/action.yaml (3)
1-2: Action Renaming and Updated DescriptionRenaming the action to
clp-core-image-runwith the new description accurately reflects its updated responsibility—executing arbitrary commands within a container image. This improvement enhances clarity and should help users understand its purpose.
29-40: Image Properties Extraction StepThe block that determines the qualified image name based on whether a published image is being used is well implemented. Confirm that the conditional supports both lowercase conversion and appropriate tagging for the image.
45-53: Docker Run Command ExecutionThe docker run step is constructed to mount the repository with the correct user and working directory, and it properly incorporates the
run_commandinput viabash -c. It would be beneficial to test this command with various input scenarios to ensure that argument splitting or spacing issues do not occur.
| @@ -1,5 +1,5 @@ | |||
| name: "clp-core-build" | |||
| description: "Builds CLP-core in the specified container" | |||
| name: "clp-core-run-on-image" | |||
There was a problem hiding this comment.
Actually I meant run-on-image by itself, without the prefix, for two reasons:
- This seems like more of a general action, not specific to clp-core.
- The "clp-core-" prefix was only ever added since we wanted "core-" as the prefix but that sounds ambiguous by itself.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/actions/run-on-image/action.yaml (2)
4-13: Clarify Input Types for Consistency
The inputs are well defined; however, note that theuse_published_imageinput is expected to hold a string value of either"true"or"false". It may be beneficial to add documentation or default values to ensure consistent use.
29-40: Image Properties Extraction is Effective
The script correctly handles the lowercase conversion for repository names when using a published image and sets the output accordingly.
Consider adding error handling in case the image name is not set or if the conversion fails, although it may not be required in this context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/run-on-image/action.yaml(1 hunks).github/workflows/clp-core-build.yaml(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (11)
.github/actions/run-on-image/action.yaml (6)
1-3: Metadata and Description are Clear
The name and description clearly convey the action’s purpose. No changes are necessary here.
15-17: Composite Action Setup Looks Good
The structure underrunswith the composite action and step definitions is set up correctly.
18-23: Artifact Download Condition is Correct
The conditional step for downloading the image artifact whenuse_published_imageis set to"false"is implemented correctly.
24-28: Image Loading Step is Appropriately Configured
The step that loads the downloaded image using Docker is clear and leverages the correct input filename.
42-44: Dependency Initialisation Step is Straightforward
The call to./tools/scripts/deps-download/init.shis simple and clear. Ensure that this script exists and has the proper execution permissions in the repository.
45-52: Docker Run Command Executes as Intended
The final step to run the specified command inside the Docker container is well composed. The use of the current user’s permissions and workspace volume mounting is correctly implemented..github/workflows/clp-core-build.yaml (5)
7-8: Include New Action File in the Trigger Paths
The added path for.github/actions/run-on-image/action.yamlin the pull_request trigger ensures that changes to the new action are tracked. This update is correct and aligns with the refactoring.
18-19: Update Trigger Paths for Push Events
Including.github/actions/run-on-image/action.yamlin the push trigger paths is appropriate so that pushes affecting the new action trigger a workflow run.
166-180: Refactored Job Step Using the New Action is Well Implemented
The centos-stream-9-binaries job now leverages the newrun-on-imageaction appropriately. The input parameters, includingimage_name,use_published_image, and the multilinerun_command, are correctly defined.
Make sure that the conditional expression foruse_published_imageevaluates correctly as intended under all circumstances.
211-223: Ubuntu Job Step Mirrors the Centre Logic Correctly
The ubuntu-jammy-binaries job’s use of therun-on-imageaction is consistent with the centos-stream-9-binaries job. Both the image name and the command execution are correctly set up, with the proper condition to determine if a published image should be used.
225-234: Binary Copy and Upload Steps Are Appropriately Conditioned
The steps for copying binaries and subsequently uploading them as artifacts are in line with the workflow’s new structure. Verify that the defined paths and output names match the expectations in downstream steps.
…n an arbitrary command on a container image. (y-scope#748) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
2 participants
Description
This PR refactors the github action
actions/clp-core-buildthat built clp-core to the actionactions/clp-core-image-runthat instead runs arbitrary bash script. This will allow the core image to be used to execute other commands/scripts that require an enviornment with all core dependencies (e.g. linters such as clang-tidy).Checklist
breaking change.
Validation performed
CI builds passing.
Summary by CodeRabbit
These improvements enhance our build reliability and testing performance, ensuring a smoother overall experience.