RHINENG-27056: update cji db-migration job with name using image_tag and suffix#2246
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the CJI db-migration ClowdJobInvocation to use a deterministic name based on IMAGE_TAG plus a random suffix, and adds a new parameter to generate that suffix so the job no longer relies on Kubernetes generateName, which is unsupported by qontract-reconcile. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Using
name: db-migration-${IMAGE_TAG}-${CJI_UID}may produce invalid Kubernetes resource names ifIMAGE_TAGcontains characters like/,:, or uppercase; consider normalizing/sanitizing the tag (e.g., replacing non-DNS-1123 characters) before interpolating it into the name. - It might be safer to bound the total length of
db-migration-${IMAGE_TAG}-${CJI_UID}(e.g., by truncating the tag portion) to avoid exceeding Kubernetes’ 63-character name limit in cases whereIMAGE_TAGis long.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `name: db-migration-${IMAGE_TAG}-${CJI_UID}` may produce invalid Kubernetes resource names if `IMAGE_TAG` contains characters like `/`, `:`, or uppercase; consider normalizing/sanitizing the tag (e.g., replacing non-DNS-1123 characters) before interpolating it into the name.
- It might be safer to bound the total length of `db-migration-${IMAGE_TAG}-${CJI_UID}` (e.g., by truncating the tag portion) to avoid exceeding Kubernetes’ 63-character name limit in cases where `IMAGE_TAG` is long.
## Individual Comments
### Comment 1
<location path="deploy/clowdapp.yaml" line_range="572" />
<code_context>
labels:
app: patchman
- generateName: db-migration-
+ name: db-migration-${IMAGE_TAG}-${CJI_UID}
spec:
appName: patchman
</code_context>
<issue_to_address>
**issue (bug_risk):** Including IMAGE_TAG in the resource name risks invalid characters or excessive length for Kubernetes object names.
Kubernetes names must be DNS-1123 compliant and ≤253 chars, but IMAGE_TAG values often contain `:`/`/` (e.g. `quay.io/...:1.2.3`), which would make this name invalid and long tags could exceed the limit. Please either sanitize/slugify IMAGE_TAG before using it here (e.g. drop the repo part and replace invalid chars) or derive a shorter, valid identifier (e.g. a hash).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| labels: | ||
| app: patchman | ||
| generateName: db-migration- | ||
| name: db-migration-${IMAGE_TAG}-${CJI_UID} |
There was a problem hiding this comment.
issue (bug_risk): Including IMAGE_TAG in the resource name risks invalid characters or excessive length for Kubernetes object names.
Kubernetes names must be DNS-1123 compliant and ≤253 chars, but IMAGE_TAG values often contain :// (e.g. quay.io/...:1.2.3), which would make this name invalid and long tags could exceed the limit. Please either sanitize/slugify IMAGE_TAG before using it here (e.g. drop the repo part and replace invalid chars) or derive a shorter, valid identifier (e.g. a hash).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2246 +/- ##
=======================================
Coverage 59.06% 59.06%
=======================================
Files 138 138
Lines 8848 8848
=======================================
Hits 5226 5226
Misses 3076 3076
Partials 546 546
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This PR:
qontract-reconciledoes not supportgenerateNamefor any k8s objectSummary by Sourcery
Update the CJI db-migration job to use an explicit name incorporating the image tag and a generated unique suffix instead of relying on Kubernetes generateName, and add a new parameter to supply that suffix.