Update operator scripts for upstream repos #200
Conversation
📝 WalkthroughWalkthroughThis PR removes the controller-bundle CI workflow, adds documentation comments to API type definitions, bumps the operator version to 0.8.1-rc1 with new Makefile targets, restructures bundle manifests by removing old generated files while updating the base ClusterServiceVersion, introduces sample resource configurations, and adds contribution automation tooling along with login service port normalization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 7
🤖 Fix all issues with AI agents
In `@controller/api/v1alpha1/exporter_types.go`:
- Around line 55-61: Update the comment on the Exporters type: change the phrase
"should be ran on a linux machine" to "should be run on a Linux machine" in the
comment block that begins "// Exporters represent..." in exporter_types.go so
the verb and proper noun are corrected.
In
`@controller/deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml`:
- Around line 45-55: The alm-example for ExporterAccessPolicy is wrong:
ExporterAccessPolicySpec defines exporterSelector and policies (not a top-level
clientSelector), so update the manifest's spec to move clientSelector into a
policies array under each Policy.From entry; specifically change the spec to
include "exporterSelector": {} and "policies": [{ "from": [{ "clientSelector":
{} }] }] to match the ExporterAccessPolicy type and the
Policy.From.clientSelector field used in exporteraccesspolicy_types.go and the
ExporterAccessPolicy resource.
In `@controller/deploy/operator/config/samples/jumpstarter_v1alpha1_client.yaml`:
- Around line 1-8: The sample YAML uses the username prefix "keycloak:..." but
the doc comment in client_types.go (referenced at the username field doc, e.g.,
the comment near the Username field/type) uses "kc:..."; update one of them so
both use the same prefix (either change the sample's spec.username to
"kc:user-name-in-keycloak" or update the doc comment to
"keycloak:user-name-in-keycloak") to ensure consistency between the sample
(spec.username) and the Username doc comment in client_types.go.
In `@controller/deploy/operator/contribute/update-contributions.sh`:
- Around line 1-2: Add strict error handling to the shell script by inserting
"set -euo pipefail" immediately after the existing shebang (#!/bin/bash) so the
script aborts on any command failure, undefined variable, or pipeline error;
this change targets the top of the script file containing the shebang line and
ensures commands like git clone, git fetch, and git commit cannot fail silently.
- Around line 24-25: The script extracts VERSION into the variable VERSION from
CSV_PATH but never validates it; add a check after the extraction in
update-contributions.sh to ensure VERSION is non-empty (e.g. if [ -z "$VERSION"
] ...), print a clear error including CSV_PATH and the failed command output,
and exit non‑zero to avoid creating branches or directories with empty names;
reference the VERSION variable and CSV_PATH used in the extraction so the check
is placed immediately after the VERSION assignment.
- Around line 42-50: Guard the subshell's cd so subsequent git remote commands
don't run if changing directory fails: update the subshell block that begins
with cd "$dir" and the git remote commands to check the cd exit (e.g., use cd
"$dir" || exit 1 or cd "$dir" && { ... } pattern) so that if cd "$dir" fails the
script aborts or skips the git remote operations; ensure the change references
the existing cd "$dir" line and the git remote get-url / set-url / add commands
inside that subshell.
- Around line 55-59: The loops iterating over COMMUNITY_OPS use subshells like
(cd "$dir" && git fetch --all && git checkout remotes/origin/main -B "$BRANCH")
without checking that cd succeeds; update those subshell commands (the ones
performing cd "$dir" before git operations) to guard the cd with || exit 1
(e.g., (cd "$dir" || exit 1 && git fetch --all && git checkout
remotes/origin/main -B "$BRANCH")) so failures to change directory abort the
script; apply the same pattern to the other similar subshell(s) in the file.
🧹 Nitpick comments (3)
controller/deploy/operator/contribute/update-contributions.sh (1)
6-6: DefaultGITHUB_USERis the PR author's personal username.This is fine for a local helper script, but other contributors will need to remember to override it. Consider either removing the default to force explicit setting, or documenting the override prominently.
controller/deploy/operator/Makefile (1)
308-311: Hardcoded OpenShift version range will become stale.The injected
v4.18-v4.21range will need manual updates as new OpenShift versions are released. Consider extracting this into a Makefile variable (e.g.,OPENSHIFT_VERSIONS) so it's easier to find and update alongsideVERSION.Proposed refactor
+# OpenShift version range for bundle metadata +OPENSHIFT_VERSIONS ?= v4.18-v4.21 + .PHONY: bundle bundle: manifests kustomize operator-sdk ## Generate bundle manifests and metadata, then validate generated files. $(OPERATOR_SDK) generate kustomize manifests -q cd config/manager && $(KUSTOMIZE) edit set image controller=$(IMG) $(KUSTOMIZE) build config/manifests | $(OPERATOR_SDK) generate bundle $(BUNDLE_GEN_FLAGS) @# Inject Red Hat OpenShift version range into bundle metadata (not handled by operator-sdk) `@if` ! grep -q 'com.redhat.openshift.versions' bundle/metadata/annotations.yaml; then \ - echo ' com.redhat.openshift.versions: v4.18-v4.21' >> bundle/metadata/annotations.yaml; \ + echo ' com.redhat.openshift.versions: $(OPENSHIFT_VERSIONS)' >> bundle/metadata/annotations.yaml; \ fi $(OPERATOR_SDK) bundle validate ./bundlecontroller/api/v1alpha1/exporteraccesspolicy_types.go (1)
53-54: Nit: capitalize sentence start.Line 54: "they help organize…" starts a new sentence after a period — should be "They help organize…".
Proposed fix
// ExporterAccessPolicies are used to define the access policies for the exporters. - // they help organize, prioritize and restrict access to the exporters by clients. + // They help organize, prioritize and restrict access to the exporters by clients.
...oller/deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
Show resolved
Hide resolved
controller/deploy/operator/config/samples/jumpstarter_v1alpha1_client.yaml
Outdated
Show resolved
Hide resolved
| #!/bin/bash | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add set -euo pipefail to catch silent failures.
Without set -e, intermediate commands (e.g., git clone, git fetch, git commit) can fail without halting the script, leading to confusing downstream errors or partial state.
Proposed fix
#!/bin/bash
+set -euo pipefail
# Target repos: "clone-url local-directory"📝 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.
| #!/bin/bash | |
| #!/bin/bash | |
| set -euo pipefail | |
🤖 Prompt for AI Agents
In `@controller/deploy/operator/contribute/update-contributions.sh` around lines 1
- 2, Add strict error handling to the shell script by inserting "set -euo
pipefail" immediately after the existing shebang (#!/bin/bash) so the script
aborts on any command failure, undefined variable, or pipeline error; this
change targets the top of the script file containing the shebang line and
ensures commands like git clone, git fetch, and git commit cannot fail silently.
There was a problem hiding this comment.
@bennyz, great! This aligns with the project's existing practice of using strict error handling in shell scripts.
🧠 Learnings used
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 339
File: packages/jumpstarter-driver-flashers/oci_bundles/build_bundle.sh:12-12
Timestamp: 2025-03-12T16:55:13.031Z
Learning: For shell scripts in the Jumpstarter project, prefer using `set -e` at the beginning of scripts to handle errors consistently throughout, rather than adding explicit error checks like `|| exit 1` for individual commands.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
controller/deploy/operator/Makefile
Outdated
| # - use the VERSION as arg of the bundle target (e.g make bundle VERSION=0.0.2) | ||
| # - use environment variables to overwrite this value (e.g export VERSION=0.0.2) | ||
| VERSION ?= 0.8.0 | ||
| VERSION ?= 0.7.5 |
There was a problem hiding this comment.
This is my attempt to release another one without using 0.8.1, but may be there is really no option and I will need to update to 0.8.1., waiting on the other repo's CI.
There was a problem hiding this comment.
The k8s one already complained, but I am less worried about that one now, waiting for the openshift one
Error: add prunes bundle jumpstarter-operator.v0.7.5 (kind-registry:5000/test-operator/jumpstarter-operator@sha256:c552d1fde9468bbf52291531a45a9586bcc426fc8bd1615f2bbfe31d7beedf77) from package jumpstarter-operator, channel alpha: this may be due to incorrect channel head (jumpstarter-operator.v0.8.0, skips/replaces []). Be aware that the head of the channel alpha where you are trying to add the jumpstarter-operator.v0.7.5 is jumpstarter-operator.v0.8.0. Upgrade graphs follows the Semantic Versioning 2.0.0 (https://semver.org/) which means that is not possible add new versions lower then the head of the channel
There was a problem hiding this comment.
so iiuc we have a version mismatch between the current operator and the current client/exporter pair?
There was a problem hiding this comment.
Yes, we will probably need to start the last release as 0.8.1 I think ... my bad, I started it
Also the 0.7.5 didn't work, gemini suggests trying 0.8.1-rc1 and then progressing RCs until we release 0.8.1
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@controller/internal/service/login/service.go`:
- Around line 134-137: Current logic blindly prepends ":" to port which corrupts
host:port values (e.g. "0.0.0.0:8086"); change the normalization in the function
that handles the local variable port so it only prepends ":" for a bare port.
Use net.SplitHostPort to detect a valid host:port pair (or parse IPv6 bracketed
form) — if net.SplitHostPort succeeds leave port as-is, otherwise if it looks
like a bare port add the ":" prefix (mirroring the behavior of ensurePort in the
operator controller). Update the block that currently checks port != "" &&
port[0] != ':' to perform this SplitHostPort-based check and only mutate the
value when it's truly a bare port.
| "--leader-elect", | ||
| "--health-probe-bind-address=:8081", | ||
| "-metrics-bind-address=:8080", | ||
| "--metrics-bind-address=:8080", |
There was a problem hiding this comment.
Hmmm the previous is probably right, let me check this detail why did cursor change it... seems unrelated.
There was a problem hiding this comment.
When I execute the manager manually
➜ jumpstarter git:(update-operator) ✗ controller/deploy/operator/bin/manager --help
Usage of controller/deploy/operator/bin/manager:
-enable-http2
If set, HTTP/2 will be enabled for the metrics and webhook servers
-health-probe-bind-address string
The address the probe endpoint binds to. (default ":8081")
-kubeconfig string
Paths to a kubeconfig. Only required if out-of-cluster.
-leader-elect
Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.
-metrics-bind-address string
The address the metrics endpoint binds to. Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service. (default "0")
-metrics-cert-key string
The name of the metrics server key file. (default "tls.key")
-metrics-cert-name string
The name of the metrics server certificate file. (default "tls.crt")
-metrics-cert-path string
The directory that contains the metrics server certificate.
-metrics-secure
If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead. (default true)
-webhook-cert-key string
The name of the webhook key file. (default "tls.key")
-webhook-cert-name string
The name of the webhook certificate file. (default "tls.crt")
-webhook-cert-path string
The directory that contains the webhook certificate.
-zap-devel
Development Mode defaults(encoder=consoleEncoder,logLevel=Debug,stackTraceLevel=Warn). Production Mode defaults(encoder=jsonEncoder,logLevel=Info,stackTraceLevel=Error) (default true)
-zap-encoder value
Zap log encoding (one of 'json' or 'console')
-zap-log-level value
Zap Level to configure the verbosity of logging. Can be one of 'debug', 'info', 'error', 'panic'or any integer value > 0 which corresponds to custom debug levels of increasing verbosity
-zap-stacktrace-level value
Zap Level at and above which stacktraces are captured (one of 'info', 'error', 'panic').
-zap-time-encoding value
Zap time encoding (one of 'epoch', 'millis', 'nano', 'iso8601', 'rfc3339' or 'rfc3339nano'). Defaults to 'epoch'.
it shows that option with a single -
| $(OPERATOR_SDK) generate kustomize manifests -q | ||
| cd config/manager && $(KUSTOMIZE) edit set image controller=$(IMG) | ||
| $(KUSTOMIZE) build config/manifests | $(OPERATOR_SDK) generate bundle $(BUNDLE_GEN_FLAGS) | ||
| @# Inject Red Hat OpenShift version range into bundle metadata (not handled by operator-sdk) |
| @@ -0,0 +1,76 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
maybe in the future we can have a github action bot or something to create the PR
There was a problem hiding this comment.
That would be cool yes, probably as soon as we get bored of doing this manually we will get motivated X-D :)
controller/deploy/operator/Makefile
Outdated
| $(KUSTOMIZE) build config/manifests | $(OPERATOR_SDK) generate bundle $(BUNDLE_GEN_FLAGS) | ||
| @# Inject Red Hat OpenShift version range into bundle metadata (not handled by operator-sdk) | ||
| @if ! grep -q 'com.redhat.openshift.versions' bundle/metadata/annotations.yaml; then \ | ||
| echo ' com.redhat.openshift.versions: v4.18-v4.21' >> bundle/metadata/annotations.yaml; \ |
There was a problem hiding this comment.
so we'll need to update it each new openshift version? 🤔
There was a problem hiding this comment.
yep, that's a manual process, we need to know which versions are going to be compatible or incompatible as our api usage progresses...
but it's a good question.
Probably we will want to make this easier over time, may be just add it to the makefile as an env var.
There was a problem hiding this comment.
+1 for the var. this way it's more visible
sidenote, already seen 5.0 nightlies cooking on the releasecontroller.. we'll probably want to play aroud with it at some point
There was a problem hiding this comment.
I wonder how 5.0 looks normally a major number bump means incompatible changes.
Those files can be regenerated anytime from our code with: ``` cd controller/deploy/operator make bundle make build-installer ```
At the same time, we are disabling the client, lease, exporter, exporterpolicy CRDs from the view of the operator, since the operator only manages the "Jumpstarter" CRDs for deployment, everything else is noise and confusion to the admin.
mangelajo
force-pushed
the
update-operator
branch
from
a3e793e to
dd3fbf5
Compare
3 participants
This PR introduces the following changes:
Executing the contribute target on this PR we get:
Summary by CodeRabbit
Documentation
Version Update
New Features
Chores