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

414 informers use best match #423

Merged
merged 2 commits into from
Dec 6, 2021
Merged

Conversation

martyspiewak
Copy link
Contributor

Closes #414

@netlify
Copy link

netlify bot commented Dec 2, 2021

✔️ Deploy Preview for elated-stonebraker-105904 canceled.

🔨 Explore the source changes: a37ccdb

🔍 Inspect the deploy log: https://app.netlify.com/sites/elated-stonebraker-105904/deploys/61ae1b35395b7e000886942c

@martyspiewak martyspiewak marked this pull request as draft December 2, 2021 20:38
@martyspiewak martyspiewak force-pushed the 414-informers-use-best-match branch from d53e66a to 95521f1 Compare December 2, 2021 20:43
@martyspiewak martyspiewak marked this pull request as ready for review December 2, 2021 20:43
@waciumawanjohi waciumawanjohi self-requested a review December 3, 2021 22:17

var selectorGetters []repository.SelectorGetter
for _, item := range scList.Items {
item := item
Copy link
Contributor

@waciumawanjohi waciumawanjohi Dec 3, 2021

Choose a reason for hiding this comment

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

Not blocking:
Is this just to create a copy of the object? Would persistentItem := item.DeepCopy() suit? Would it be more grokkable?

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 think that would work too but I was trying to be consistent with the way the controller finds the best matching SC (see here)

Copy link
Contributor

@waciumawanjohi waciumawanjohi left a comment

Choose a reason for hiding this comment

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

Just the one change to fix the test. Otherwise, lgtm!

@martyspiewak martyspiewak force-pushed the 414-informers-use-best-match branch from 95521f1 to a37ccdb Compare December 6, 2021 14:16
@martyspiewak martyspiewak merged commit d7324e9 into main Dec 6, 2021
@martyspiewak martyspiewak deleted the 414-informers-use-best-match branch December 6, 2021 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Informers still use exact match to find workloads/deliverables for supply chains/deliveries
2 participants