Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix job annotation overwrite #1612

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Fix job annotation overwrite #1612

merged 2 commits into from
Dec 3, 2024

Conversation

RafalKorepta
Copy link
Contributor

aac3d49

Fixed ability to overwrite annotation and labels in Job metadata

b483c4b

Remove none existent post-upgrade-job values
Reference
https://github.com/redpanda-data/helm-charts/releases/tag/redpanda-5.9.6
#1534

helm-charts/CHANGELOG.md

Lines 85 to 92 in 05d3182

#### Removed
* `post_upgrade_job.*`, and the post-upgrade job itself, has been removed. All
it's functionality has been consolidated into the `post_install_job`, which
actually runs on both post-install and post-upgrade.
The consolidated job now runs the redpanda-operator image, which may be
controlled the same way as the additional controllers:
`statefulset.controllers.{image,repository}`.

Copy link
Contributor

@andrewstucki andrewstucki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the entire context of this, but made some comments and suggestions about wording for at least the changelog if the desire is to add annotations/labels v. overriding them.

CHANGELOG.md Outdated Show resolved Hide resolved
),
Annotations: helmette.Merge(
helmette.Default(map[string]string{}, values.PostInstallJob.Annotations),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing that we have this change since we don't want others to be able to override the helm-specific annotations?

Is there a perceived use-case where we want users to be able to override these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing that we have this change since we don't want others to be able to override the helm-specific annotations?

It's other way around. If someone would like to override (not recommended) with this change they will be able to change annotation. Merge from mergo works other way around.

Is there a perceived use-case where we want users to be able to override these?

As in the https://redpandacommunity.slack.com/archives/C01AJDUT88N/p1732833410021669 community member would like to delete Job after they succeed.

helmette.Default(map[string]string{}, values.PostInstallJob.Labels),
FullLabels(dot),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this change if PostInstallJob.Labels is considered an override then shouldn't we allow for overriding FullLabels(dot)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change does exactly that. It allows for overriding. I will add test case to assert that generated label by the FullLabel function will be changed if I provide in the same key different label value.

#### Added
#### Changed
#### Fixed
* ability to overwrite annotation and labels in Job metadata
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the current implementation isn't really for overriding, but adding additional labels/annotations that we normally wouldn't set? Basically:

# -- Additional labels to apply to the Pods of this Job.
labels: {}
# -- Additional annotations to apply to the Pods of this Job.
annotations: {}

If so, then I think the implementation is fine, but this should be something like:

Suggested change
* ability to overwrite annotation and labels in Job metadata
* ability to set additional annotation and labels in Job metadata

Copy link
Contributor Author

@RafalKorepta RafalKorepta Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change tries to change the set additional to overwrite even the default one.

The comment references the podTemplate

podTemplate:
# -- Additional labels to apply to the Pods of this Job.
labels: {}
# -- Additional annotations to apply to the Pods of this Job.
annotations: {}
which applies to Job.Spec.Template. This change is focusing on Job.Metadata.[Labels/Annotation].

@RafalKorepta RafalKorepta merged commit 926cfe0 into main Dec 3, 2024
43 checks passed
@RafalKorepta RafalKorepta deleted the rk/job-helm-hooks branch December 3, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants