Skip to content

Move pkg/predicate closer to where it is used.#3491

Closed
jmrodri wants to merge 2 commits intooperator-framework:masterfrom
jmrodri:move-predicate
Closed

Move pkg/predicate closer to where it is used.#3491
jmrodri wants to merge 2 commits intooperator-framework:masterfrom
jmrodri:move-predicate

Conversation

@jmrodri
Copy link
Copy Markdown
Member

@jmrodri jmrodri commented Jul 22, 2020

Description of the change:
Move pkg/predicate to the ansible package because it is not used anywhere else.

Motivation for the change:
Since this package is not used outside the ansible package, it makes sense to have it co-located. In the future we hope to move it to operator-lib or controller-runtime.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Copy link
Copy Markdown
Member

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2020
Copy link
Copy Markdown
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@joelanford
Copy link
Copy Markdown
Member

Just double checking... Is the upstream predicate the same as the one the ansible operator was using? The ansible e2e test keeps failing.

@jmrodri
Copy link
Copy Markdown
Member Author

jmrodri commented Jul 23, 2020

GenerationChangedPredicate is slightly different between the 2 implementations:

The one in SDK https://github.com/operator-framework/operator-sdk/blob/master/pkg/predicate/predicate.go#L56-L59:

	if e.MetaNew.GetGeneration() == e.MetaOld.GetGeneration() && e.MetaNew.GetGeneration() != 0 {
		return false
	}
	return true

And the one from controller-runtime is simpler https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/predicate/predicate.go#L181:

	return e.MetaNew.GetGeneration() != e.MetaOld.GetGeneration()

So looks like by using the upstream version we introduced a regression fixed by PR #2830 and identified by Issue #2108.

@jmrodri
Copy link
Copy Markdown
Member Author

jmrodri commented Jul 24, 2020

Closing in favor of PR #3532

@jmrodri jmrodri closed this Jul 24, 2020
@jmrodri jmrodri deleted the move-predicate branch August 17, 2020 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants