eco_gotests: build container image #826
Conversation
📝 WalkthroughWalkthroughAdds an optional local build step for the eco_gotests container, updates the role README with new variables and requirements, bumps several GitHub Actions checkout/setup action versions, and adjusts two CI helper scripts for summary output and role-path normalization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
roles/eco_gotests/tasks/build.yml (1)
3-10: Consider addingchanged_whento track state accurately.The build task will always report as changed even if the image already exists and no rebuild occurred. Adding a
changed_whencondition can make the playbook output more meaningful for idempotency tracking.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
roles/eco_gotests/README.md(3 hunks)roles/eco_gotests/tasks/build.yml(1 hunks)roles/eco_gotests/tasks/main.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dci/check
🔇 Additional comments (2)
roles/eco_gotests/tasks/main.yml (1)
13-16: LGTM!The conditional include is cleanly structured and properly gates the build step. Good placement in the workflow.
roles/eco_gotests/README.md (1)
1-72: LGTM!The README updates clearly document the new optional build workflow and variable precedence. The reorganized structure with explicit Required Variables section improves clarity.
fredericlepied
force-pushed
the
eco-gotests
branch
from
57b210a to
a49d23a
Compare
fredericlepied
force-pushed
the
eco-gotests
branch
from
a49d23a to
a2a919b
Compare
|
from change #826: |
fredericlepied
force-pushed
the
eco-gotests
branch
from
a2a919b to
8447ec1
Compare
|
from change #826: |
fredericlepied
force-pushed
the
eco-gotests
branch
from
8447ec1 to
f5e5177
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
roles/eco_gotests/README.md (1)
22-22: Fix variable name typo:kubconfig→kubeconfig.Lines 22 and 53 reference
eco_gotests_kubconfig_dir, which appears to be a typo. The standard Kubernetes terminology iskubeconfig(one word). Please correct this toeco_gotests_kubeconfig_dirfor consistency and clarity.-- `eco_gotests_kubconfig_dir`: Directory containing kubeconfig files +- `eco_gotests_kubeconfig_dir`: Directory containing kubeconfig files- eco_gotests_kubconfig_dir: /home/user/clusterconfigs + eco_gotests_kubeconfig_dir: /home/user/clusterconfigsAlso applies to: 53-53
🧹 Nitpick comments (2)
roles/eco_gotests/README.md (1)
29-29: Minor: clarifyeco_gotests_pathdescription.The description reads "path where to find the source code to build the container." Consider adding a note that the directory should contain a Dockerfile or equivalent build definition, to help users understand what structure is expected.
hack/check_version.sh (1)
50-50: Extract the sed expression to reduce duplication.The identical sed pipeline appears twice—once for REMOVED and once for ADDED. Extracting this into a helper function or variable would improve maintainability and reduce the risk of inconsistencies if the normalization logic needs to change.
Example refactor:
normalize_role_path() { sed -Ee 's@/(defaults|templates|vars|scripts|handlers|tests|meta|tasks|files)/@/@g' -e 's@/$@@' -e 's@^roles/@@' }Then update lines 50 and 53:
-sed -Ee 's@/(defaults|templates|vars|scripts|handlers|tests|meta|tasks|files)/@/@g' -e 's@/$@@' -e 's@^roles/@@' | uniq) +normalize_role_path | uniq)Also applies to: 53-53
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/dci-merge.yml(1 hunks).github/workflows/pr.yml(4 hunks).github/workflows/preflight-release.yml(1 hunks).github/workflows/publish.yml(1 hunks)hack/check_doc.sh(1 hunks)hack/check_version.sh(1 hunks)roles/eco_gotests/README.md(3 hunks)roles/eco_gotests/tasks/build.yml(1 hunks)roles/eco_gotests/tasks/main.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/preflight-release.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- roles/eco_gotests/tasks/build.yml
- roles/eco_gotests/tasks/main.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dci/check
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Sanity Check (stable-2.18)
🔇 Additional comments (7)
roles/eco_gotests/README.md (1)
1-57: Documentation structure and examples look good.The README now clearly separates Required and Optional Variables, includes a new build-path option, and provides helpful examples. The Requirements section is explicit about Podman and image access. The wording fix for ptp-operator installation is appreciated.
hack/check_version.sh (1)
49-53: Path normalization logic is solid.The reworked sed expressions correctly normalize role paths by collapsing special directory segments into a single slash, then cleaning up trailing and leading delimiters. This ensures consistent role name extraction regardless of file depth within the role directory structure.
hack/check_doc.sh (1)
18-20: Output destination now visible in local/non-GHA contexts.Redirecting to
/dev/stdoutinstead of/dev/nullwhenGITHUB_STEP_SUMMARYis unset makes script output visible during local development and testing. This improves observability outside GitHub Actions workflows where the env var wouldn't be set..github/workflows/pr.yml (2)
13-13: Good: Consistent checkout action updates across jobs.The v6 checkout reference is applied uniformly to the
ansible-lint,sanity, andcheck-docsjobs, which is good for maintainability and consistency.Also applies to: 98-98, 143-143
13-13: Action version pins are stable and compatible; clarify depends-on-action comment.The action versions are correctly pinned and compatible:
actions/[email protected]andactions/[email protected]are fully compatible with each other and Python 3.11 on GitHub-hosted runners.depends-on/depends-on-actionis pinned to a specific commit SHA (826c14...), which is actually a stable security practice—despite the# maincomment suggesting otherwise. The SHA pin is immutable and as stable as (or more stable than) a release tag. If the intent was to indicate the source branch, clarify the comment to match the SHA usage; otherwise, the current pin is secure..github/workflows/dci-merge.yml (1)
14-16: No compatibility issues found; configuration is correct and necessary.The upgrade to checkout v6.0.0 with
fetch-depth: 0is appropriate and properly configured. The dci-merge.sh script explicitly requires full commit history—it usesgit log --merges "${BASE_SHA}".."${HEAD_SHA}"(line 48) andgit diff --name-only "$BASE_SHA" "$HEAD_SHA"(line 93), both of which require the complete history to function. Thefetch-depth: 0setting is essential, not optional. Checkout v6 is widely used across other workflows in the repository and maintains full compatibility with thefetch-depthparameter. No bos2-specific infrastructure concerns exist with this standard GitHub Action..github/workflows/publish.yml (1)
12-12: Based on the web search results and examination of the workflow, I can now provide a comprehensive assessment.The key breaking change in v6.0.0 is the relocation of persisted credentials from
.git/configto a separate file under$RUNNER_TEMP. This is more secure but requires Actions Runner >= v2.329.0.Analysis of the publish.yml workflow:
- The workflow uses git operations (
git show) to extract commit metadata on lines 17-19- No explicit
persist-credentialsparameter is set, meaning the default (true) applies—credentials will be persisted- The workflow does not package the repository or credentials into artifacts/containers
- Runner is
ubuntu-24.04, which is recent and should ship with runner v2.329.0 or later- The credential storage change is transparent to standard git operations; they will work correctly regardless of where the token file is stored
Conclusion:
The upgrade to v6.0.0 is compatible with this workflow. The breaking changes do not affect how this workflow uses the checkout action.
No compatibility issues found with the actions/checkout v6.0.0 upgrade. The persisted credentials storage change is transparent to the git operations performed in the workflow, and the ubuntu-24.04 runner meets the minimum version requirement (v2.329.0).
|
from change #826: |
|
from change https://github.com/dci-labs/bos2-ci-config/pull/599:
|
SUMMARY
Add an optional build step to the eco_gotests role. This will enable testing PR from the eco_gotests repository.
ISSUE TYPE
Tests