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

Add generate bundle --ignore-if-only-created-at-changed option #6419

Closed
wants to merge 2 commits into from

Conversation

kaovilai
Copy link

@kaovilai kaovilai commented May 4, 2023

Signed-off-by: Tiger Kaovilai tkaovila@redhat.com

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:

2. We can define a constant. As part of the testdata generation process we replace the createdAt value in the generated CSV with this constant. This should prevent the git diff check from failing solely because of the createdAt value.

Number 2 probably makes more sense then. I assume the constant and code changes for the testdata generation should be under the hack/ directory?

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 if config/ has become out of sync with bundle/ by running make 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:

Fix #6285

@kaovilai
Copy link
Author

kaovilai commented May 4, 2023

/cc @redhatrises

@openshift-ci
Copy link

openshift-ci bot commented May 4, 2023

@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:

/cc @redhatrises

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.

@kaovilai kaovilai force-pushed the fix6285 branch 4 times, most recently from 5be0ea2 to 88a1c9d Compare May 4, 2023 14:38
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@kaovilai kaovilai requested a review from ncdc May 9, 2023 14:45
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@kaovilai kaovilai changed the title Add generate bundle --ignore-if-only-createdAt option Add generate bundle --ignore-if-only-created-at-changed option May 9, 2023

// 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 {
Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

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

+1

Comment on lines +163 to +170
// 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"]
Copy link
Member

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?

Copy link
Author

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!

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 4, 2023
@kaovilai
Copy link
Author

/lifecycle frozen

@openshift-ci
Copy link

openshift-ci bot commented Sep 27, 2023

@kaovilai: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/lifecycle frozen

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.

@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 28, 2023
@kaovilai
Copy link
Author

/lifecycle frozen

@openshift-ci
Copy link

openshift-ci bot commented Oct 29, 2023

@kaovilai: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/lifecycle frozen

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.

@kaovilai
Copy link
Author

/remove-lifecycle rotten

@openshift-ci openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 30, 2023
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 29, 2024
@ncdc ncdc removed their request for review January 29, 2024 17:41
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 29, 2024
@kaovilai
Copy link
Author

closing as no longer working on this. takeover as necessary.

@kaovilai kaovilai closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bundle: createdAt field is always updated when make bundle is run
4 participants