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

Added labelFilter for source CRD #1461

Merged
merged 4 commits into from
Sep 24, 2020
Merged

Added labelFilter for source CRD #1461

merged 4 commits into from
Sep 24, 2020

Conversation

JoaoBraveCoding
Copy link
Contributor

@JoaoBraveCoding JoaoBraveCoding commented Mar 6, 2020 •
edited

Fixes #1450

Added labelFilter flag for source CRD and added tests to test new behaviour.

Signed-off-by: João Marçal joao.marcal12@gmail.com

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2020
@JoaoBraveCoding
Copy link
Contributor Author

/assign @linki

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2020
njuettner
njuettner requested changes Mar 17, 2020
View reviewed changes
Copy link
Member

@njuettner njuettner left a comment

Choose a reason for hiding this comment

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

Thanks for you PR, added some nits. PTAL.

source/crd_test.go Outdated Show resolved Hide resolved
source/crd.go Outdated Show resolved Hide resolved
@njuettner
Copy link
Member

And maybe it makes sense to mention it is only used by using a CRD as source?

@JoaoBraveCoding
Copy link
Contributor Author

And maybe it makes sense to mention it is only used by using a CRD as source?

Added comment that currently only source CRD is supported in the Flag help

@JoaoBraveCoding
Copy link
Contributor Author

@njuettner it seems like Travis CI build is failing because of OVH provider pushed yesterday to master. I already tried to rebase but it doesn't find the definition for DomainFilter

@njuettner
Copy link
Member

yes @JoaoBraveCoding can you do a rebase please? this should be fixed now

@JoaoBraveCoding
Copy link
Contributor Author

@njuettner Should be ready for merging :)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2020
@JoaoBraveCoding
Copy link
Contributor Author

@njuettner @linki @Raffo @hjacobs could we get this merged? It has already been reviewed and fixed.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

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

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 13, 2020
@seanmalloy seanmalloy mentioned this pull request Aug 14, 2020
Closed
@seanmalloy
Copy link
Member

/remove-lifecycle stale
/kind feature

@JoaoBraveCoding can you rebase this PR one more time and fix the conflict in file source/crd.go?

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 14, 2020
@seanmalloy
Copy link
Member

/assign

seanmalloy
seanmalloy requested changes Sep 1, 2020
View reviewed changes
pkg/apis/externaldns/types.go Outdated Show resolved Hide resolved
@vinny-sabatini
Copy link
Contributor

/unassign @seanmalloy
/assign

vinny-sabatini
vinny-sabatini reviewed Sep 16, 2020
View reviewed changes
Copy link
Contributor

@vinny-sabatini vinny-sabatini left a comment

Choose a reason for hiding this comment

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

It looks like there was one requested change from @njuettner that may have been missed and has been marked resolved (ref comment in source/crd.go), but other than that this looks good to me.

If you could either make that change, or add a comment noting why you do not believe it's necessary, I can throw my LGTM label on the PR and will work with the official maintainers to get this reviewed and hopefully merged soon! Thanks for the PR!

source/crd.go Outdated Show resolved Hide resolved
vinny-sabatini
vinny-sabatini suggested changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@vinny-sabatini vinny-sabatini left a comment

Choose a reason for hiding this comment

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

Thanks for fixing that formatting. I did notice one last little detail to tweak around formatting, sorry I missed it on the last review. I gave the PR one last review and the rest looks good to me.

source/crd_test.go Outdated Show resolved Hide resolved
@JoaoBraveCoding @vinny-sabatini
Update source/crd_test.go
56e62d0 
Co-authored-by: Vinny Sabatini <vincent.sabatini@gmail.com>
vinny-sabatini
vinny-sabatini approved these changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@vinny-sabatini vinny-sabatini left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning that up, I will try to work with the official maintainers to get this reviewed and hopefully merged.
/lgtm
/assign @Raffo @njuettner

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2020
@Raffo
Copy link
Contributor

Raffo commented Sep 24, 2020

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2020
@Raffo Raffo added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 24, 2020
@Raffo
Copy link
Contributor

Raffo commented Sep 24, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoaoBraveCoding, Raffo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2020
@k8s-ci-robot k8s-ci-robot merged commit 79ea648 into kubernetes-sigs:master Sep 24, 2020
@JoaoBraveCoding JoaoBraveCoding deleted the crd-labels branch September 24, 2020 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanmalloy seanmalloy seanmalloy requested changes

@vinny-sabatini vinny-sabatini vinny-sabatini approved these changes

@linki linki Awaiting requested review from linki

@njuettner njuettner Awaiting requested review from njuettner

Assignees

@linki linki

@Raffo Raffo

@njuettner njuettner

@vinny-sabatini vinny-sabatini

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add label filtering to source CRD
8 participants
@JoaoBraveCoding @njuettner @fejta-bot @seanmalloy @vinny-sabatini @Raffo @k8s-ci-robot @linki