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 duplicate definition of predicate #562

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Conversation

davidjumani
Copy link
Contributor

@davidjumani davidjumani commented Sep 19, 2024

Fixes the issue of multiple declarations of Predicate when multiple snapshots are generated

This causes compilation issues after running codegen on the latest v0.35.x

Eg: In gloo OSS when multiple snapshots are created within the same package (discovery snapshot, setup snapshot), multiple Predicate definitions are generated as this is in the _template file.
This moves it to the core package to resolve this issue

@solo-changelog-bot
Copy link

Issues linked to changelog:
solo-io/gloo#9274

Copy link
Contributor

@sam-heilbron sam-heilbron left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -17,3 +17,5 @@ func (m *Metadata) Ref() *ResourceRef {
func (m *Metadata) Match(ref *ResourceRef) bool {
return m.GetNamespace() == ref.GetNamespace() && m.GetName() == ref.GetName()
}

type Predicate func(metadata *Metadata) bool
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since this is in the core package, core.Predicate is kind of generic thing. Perhaps we clarify this is a MetadataPredicate?

- type: FIX
issueLink: https://github.com/solo-io/gloo/issues/9274
resolvesIssue: false
description: Fixes the issue of multiple declarations of Predicate when multiple snapshots are generated
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @nfuden 's point in Slack, could we either inclue the error we see in the PR body, or add a test to show that we can use this Predicate (which would prove that it would work in gloo as well)?

@soloio-bulldozer soloio-bulldozer bot merged commit dc4646f into v0.35.x Sep 19, 2024
2 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the fix-predicate branch September 19, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants