ROX-33117: support for operator chart publishing#19107
Conversation
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at 284f4ce. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19107 +/- ##
=======================================
Coverage 49.25% 49.25%
=======================================
Files 2725 2725
Lines 205582 205582
=======================================
+ Hits 101261 101267 +6
+ Misses 96784 96780 -4
+ Partials 7537 7535 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0ec26ea to
aa756c7
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
publish-helm-charts.shscript now hard-requires anoperator_chart_dirargument; if you expect any reuse of this script outside the updatedpush_helm_chartspath, consider making the operator chart parameter optional (with a clear conditional branch) to avoid surprising failures in other callers. - In
push_helm_charts, you now create an additionalmktempdirectory for the operator chart but never clean it up; adding a simpletrap 'rm -rf ...' EXITto remove all temporary directories would keep the script from leaking temp data across repeated runs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `publish-helm-charts.sh` script now hard-requires an `operator_chart_dir` argument; if you expect any reuse of this script outside the updated `push_helm_charts` path, consider making the operator chart parameter optional (with a clear conditional branch) to avoid surprising failures in other callers.
- In `push_helm_charts`, you now create an additional `mktemp` directory for the operator chart but never clean it up; adding a simple `trap 'rm -rf ...' EXIT` to remove all temporary directories would keep the script from leaking temp data across repeated runs.
## Individual Comments
### Comment 1
<location path="operator/Makefile" line_range="326-328" />
<code_context>
chart: kubebuilder manifests ## Generate a helm chart with all necessary resources.
# The dependency above makes sure protos are up to date, so we can skip this time-consuming process below
# by specifying the SKIP env var. Otherwise each target that kubebuilder invokes (and there is a bunch) would regen protos.
+ if [ "$(ROX_IMAGE_FLAVOR)" = opensource ]; then sed -i'.bak' -e 's,^projectName: rhacs-operator,projectName: stackrox-operator,' PROJECT; fi
ROX_OPERATOR_SKIP_PROTO_GENERATED_SRCS=true $(KUBEBUILDER) edit --plugins=helm/v2-alpha --force
+ if [ "$(ROX_IMAGE_FLAVOR)" = opensource ]; then mv PROJECT.bak PROJECT; fi
sed -i'.bak' -e 's,0.1.0,$(VERSION),g' dist/chart/Chart.yaml
rm -f dist/chart/Chart.yaml.bak
</code_context>
<issue_to_address>
**issue:** Guard against leaving PROJECT mutated if the kubebuilder step fails mid-target.
Because `PROJECT` is edited in place for `ROX_IMAGE_FLAVOR=opensource`, a failure in `$(KUBEBUILDER) edit ...` will skip the restore step and leave `PROJECT` in the temporary state. To avoid leaving the repo dirty, consider running kubebuilder against a copy of `PROJECT` or adding a trap/cleanup so the original is always restored on failure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The updated
push_helm_charts/publish-helm-charts.shsignatures now hard-require operator chart inputs; if these scripts are reused in any other context (e.g., manual runs, older branches, or ad‑hoc tooling), consider making the operator chart arguments optional with sensible fallbacks to preserve backward compatibility. - In
.github/workflows/build.yaml, the operator chart tar step usestar -cvzfwhich can produce very verbose logs; unless you rely on that output, consider dropping the-vflag to keep CI logs smaller and easier to scan.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The updated `push_helm_charts`/`publish-helm-charts.sh` signatures now hard-require operator chart inputs; if these scripts are reused in any other context (e.g., manual runs, older branches, or ad‑hoc tooling), consider making the operator chart arguments optional with sensible fallbacks to preserve backward compatibility.
- In `.github/workflows/build.yaml`, the operator chart tar step uses `tar -cvzf` which can produce very verbose logs; unless you rely on that output, consider dropping the `-v` flag to keep CI logs smaller and easier to scan.
## Individual Comments
### Comment 1
<location path="operator/Makefile" line_range="326" />
<code_context>
chart: kubebuilder manifests ## Generate a helm chart with all necessary resources.
# The dependency above makes sure protos are up to date, so we can skip this time-consuming process below
# by specifying the SKIP env var. Otherwise each target that kubebuilder invokes (and there is a bunch) would regen protos.
+ if [ "$(ROX_IMAGE_FLAVOR)" = opensource ]; then sed -i'.bak' -e 's,^projectName: rhacs-operator,projectName: stackrox-operator,' PROJECT; fi
ROX_OPERATOR_SKIP_PROTO_GENERATED_SRCS=true $(KUBEBUILDER) edit --plugins=helm/v2-alpha --force
+ if [ "$(ROX_IMAGE_FLAVOR)" = opensource ]; then mv PROJECT.bak PROJECT; fi
sed -i'.bak' -e 's,0.1.0,$(VERSION),g' dist/chart/Chart.yaml
rm -f dist/chart/Chart.yaml.bak
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The conditional PROJECT mutation is somewhat brittle on the exact `projectName` format.
This depends on `PROJECT` containing exactly `projectName: rhacs-operator` with no indentation, so any upstream formatting change (whitespace, key order, etc.) would cause the `sed` to no-op and leave the `rhacs-operator` name in the opensource chart. Please either make the match more tolerant (e.g., allow leading whitespace) or move the flavor-specific `projectName` into a templated/derived file instead of editing the kubebuilder-generated `PROJECT` in place.
```suggestion
if [ "$(ROX_IMAGE_FLAVOR)" = opensource ]; then sed -i'.bak' -e 's,^[[:space:]]*projectName:[[:space:]]*rhacs-operator,projectName: stackrox-operator,' PROJECT; fi
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
I mark this review as "Request changes" because the used approach seems the one that puts least resistance during implementation above the experience of ourselves who'd have to use it.
How do I want this to be changed? roxctl helm output is a good example of how things could be. It's not necessary we plug into that but I'd say we deserve something better than having to run ROX_IMAGE_FLAVOR=development_build make -C operator chart as a step to deploy the product.
08e4c8c to
150f393
Compare
msugakov
left a comment
There was a problem hiding this comment.
Small things but I felt I need to comment on them. The primary reason I'm not approving is that we converge on #19107 (comment)
446fb4e to
284f4ce
Compare
|
/retest |
1 similar comment
|
/retest |
Description
Update the release pipeline for the operator chart for both the community and downstream flavor.
I think this change should be sufficient, because I don't see any chart specific logic that needs updating in the release-artifacts repo, nor in the downstream mirror job, but maybe I'm not aware of everything since the docs seem to be somewhat outdated.
FWIW updating the README in https://github.com/stackrox/helm-charts will be covered by a dedicated ticket.
Also the chart itself will get some additional polish (description, icon link, etc), but that can be done independently of this PR.
Docs:
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Tested in https://github.com/stackrox/release-artifacts/pull/210 using these additional changes to make it as similar as possible to the real thing: