-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add generate bundle --ignore-if-only-created-at-changed
option
#6419
Conversation
/cc @redhatrises |
@kaovilai: GitHub didn't allow me to request PR reviews from the following users: redhatrises. Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
5be0ea2
to
88a1c9d
Compare
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
generate bundle --ignore-if-only-createdAt
optiongenerate bundle --ignore-if-only-created-at-changed
option
|
||
// If a reader is set, and there is a flag to not update createdAt, then | ||
// set the CSV's createdAt to the previous CSV's createdAt if its the only change. | ||
if g.ignoreIfOnlyCreatedAtChanged && g.getReader != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't happen of overwrite
is set to false. If overwrite is explicitly set to false, and ignoreIfOnlyCreatedAtChanged
is passed then we should possibly error out. If not, that defeats the purpose of us tracking createdAt
to see the latest changes in bundles. It would be helpful to add that check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
// Set WebhookDefinitions if nil to avoid diffing on it | ||
if prevCSV.Spec.WebhookDefinitions == nil { | ||
prevCSV.Spec.WebhookDefinitions = []v1alpha1.WebhookDescription{} | ||
} | ||
if csvWithoutCreatedAtChange.ObjectMeta.Annotations == nil { | ||
csvWithoutCreatedAtChange.ObjectMeta.Annotations = map[string]string{} | ||
} | ||
csvWithoutCreatedAtChange.ObjectMeta.Annotations["createdAt"] = prevCSV.ObjectMeta.Annotations["createdAt"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have the code for walking through the input directories, collecting the manifests and unmarshalling it into the CSV in here. We should be able to fetch the createdAt at that step from annotations, and accordingly set it in here. This would avoid us doing the comparisons with webhooks done below.
Another option is, once we identify the CSV manifest, we can unmarshal it into a runtime.Object
as done in this PR, and pass it on through the collector, to apply it in here.
That would make sure that we are performing all operations related to inputs being passed on through flags in place and not read the input CSV multiple times.
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds more efficient. Will have a look. Thanks!
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
/lifecycle frozen |
@kaovilai: The In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
/lifecycle frozen |
@kaovilai: The In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/remove-lifecycle rotten |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
closing as no longer working on this. takeover as necessary. |
Signed-off-by: Tiger Kaovilai [email protected]
Description of the change:
Add ability to ignore bundle updates on
generate bundle
command if createdAt timestamp is the only change.The flag to use is
--ignore-if-only-created-at-changed
.Motivation for the change:
from PR
This is indeed a hack. A constant only works for test data. For a real operators repo, we would want the createdAt updated when there are changes in
config/
. It should be possible to simply in CI check ifconfig/
has become out of sync withbundle/
by runningmake bundle
in CI and diffing the bundle without hacky workarounds noted in #6285.The only other explanation that would make sense for not honoring this issue is if the suggestion is to add
bundle/
to.gitignore
which further hides the resulting bundle CSV from version control and makes it harder to track down issues.Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments
(seechangelog/fragments/00-template.yaml
)website/content/en/docs
Fix #6285