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.

Sign up for GitHub

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

Jump to bottom

fix: only wait for changeset if the result is not empty #4296

Merged
merged 1 commit into from
Oct 12, 2023
Merged

fix: only wait for changeset if the result is not empty #4296

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions internal/utils/apply.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@ func Apply(ctx context.Context, rcg genericclioptions.RESTClientGetter, opts *ru
changeSet.Append(cs.Entries)
}

if err := waitForSet(rcg, opts, changeSet); err != nil {
return "", err
if len(changeSet.Entries) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this shouldn't be rather be made part of ResourceManager#WaitForSet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I think if you want to wait you would want to wait for something and not wait for whatever even if there is nothing there. This makes more sense here readability wise.

Ultimately I don't care where this check lives. 😊

Copy link
Member

Choose a reason for hiding this comment

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

If I call ResourceManager#WaitForSet with an empty set and it doesn't immediately return (because there's nothing to wait for) this is a bug in that method IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would you call Wait if there is nothing to wait for? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's like, I don't expect any visitors, but I open the door just in case. :D

Copy link
Member

Choose a reason for hiding this comment

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

A function that behaves indeterministically if called with certain input is a recipe for disaster. Be liberal in what you accept. But I really don't mind in this specific case so just leaving this here as a general remark.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should look into the logic here and see where it hangs: https://github.com/fluxcd/pkg/blob/main/ssa/manager_wait.go But also the check in this PR is good, no need to instantiate a whole resource manager if there is nothing to wait for.

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 can do both if that's okay with you two?!

if err := waitForSet(rcg, opts, changeSet); err != nil {
return "", err
}
}

if len(stageTwo) > 0 {
Expand Down