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

test: implement e2e test of the deny-label-ns flag #1070

Merged
merged 1 commit into from
Mar 10, 2023
Merged

test: implement e2e test of the deny-label-ns flag #1070

merged 1 commit into from
Mar 10, 2023

Conversation

AhmedGrati
Copy link

@AhmedGrati AhmedGrati commented Mar 2, 2023 •
edited
Loading

Resolves #1060.
Tested scenarios:

  • Wildcard denied namespace
  • Normal denied namespace
  • Wildcard denied namespace but allow some namespaces using the -extra-label-ns flag
  • *.kubernetes.io namespace is by default denied.

@netlify
Copy link

netlify bot commented Mar 2, 2023 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit 16abfd7
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/640b026218fa740007f3176f
😎 Deploy Preview https://deploy-preview-1070--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 2, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @AhmedGrati. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot requested review from adrianchiris and kad March 2, 2023 09:40
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 2, 2023
@AhmedGrati
Copy link
Author

/cc @marquiz

@k8s-ci-robot k8s-ci-robot requested a review from marquiz March 2, 2023 09:43
marquiz
marquiz reviewed Mar 3, 2023
View reviewed changes
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Thanks @AhmedGrati for the PR. And, really good to get some coverage for the extra-label-ns flag, too. Just a few comments but generally looks good

test/e2e/node_feature_discovery_test.go Outdated Show resolved Hide resolved
test/e2e/node_feature_discovery_test.go Outdated Show resolved Hide resolved
test/e2e/node_feature_discovery_test.go Show resolved Hide resolved
@marquiz
Copy link
Contributor

marquiz commented Mar 3, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 3, 2023
@AhmedGrati AhmedGrati force-pushed the feat-implement-e2e-denylabelns-flag branch from 7d2fddd to 8b38918 Compare March 4, 2023 21:44
@AhmedGrati AhmedGrati requested review from marquiz and removed request for kad and adrianchiris March 4, 2023 22:08
marquiz
marquiz reviewed Mar 6, 2023
View reviewed changes
test/e2e/node_feature_discovery_test.go Outdated Show resolved Hide resolved
test/e2e/node_feature_discovery_test.go Outdated Show resolved Hide resolved
test/e2e/node_feature_discovery_test.go Outdated Show resolved Hide resolved
@AhmedGrati AhmedGrati force-pushed the feat-implement-e2e-denylabelns-flag branch from 8b38918 to 932c75c Compare March 6, 2023 19:33
@AhmedGrati AhmedGrati requested a review from marquiz March 6, 2023 19:50
@AhmedGrati AhmedGrati force-pushed the feat-implement-e2e-denylabelns-flag branch from 932c75c to ba1ad9e Compare March 6, 2023 23:01
@AhmedGrati AhmedGrati requested a review from marquiz March 6, 2023 23:01
@marquiz
Copy link
Contributor

marquiz commented Mar 7, 2023

I tried to run these and hit some issues. We need #1074 before merging this one

@marquiz
Copy link
Contributor

marquiz commented Mar 7, 2023

@AhmedGrati please rebase (and address the one open comment). After that we should be good to go

@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 7, 2023
@AhmedGrati
Copy link
Author

@marquiz which comment?

@AhmedGrati AhmedGrati force-pushed the feat-implement-e2e-denylabelns-flag branch from ba1ad9e to d8764b1 Compare March 7, 2023 12:48
@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 7, 2023
@marquiz
Copy link
Contributor

marquiz commented Mar 7, 2023

which comment?

Woops, my review was pending, not submitted 🤦

@AhmedGrati AhmedGrati force-pushed the feat-implement-e2e-denylabelns-flag branch from d8764b1 to 0295dc0 Compare March 7, 2023 18:48
@AhmedGrati AhmedGrati requested a review from marquiz March 7, 2023 18:48
marquiz
marquiz approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Great work @AhmedGrati on this! I'm ready to get this in

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2023
@marquiz
Copy link
Contributor

marquiz commented Mar 8, 2023

/assign @fmuyassarov

@AhmedGrati AhmedGrati force-pushed the feat-implement-e2e-denylabelns-flag branch from 0295dc0 to 0c80e0b Compare March 9, 2023 18:52
@AhmedGrati AhmedGrati requested review from marquiz and fmuyassarov and removed request for marquiz and fmuyassarov March 9, 2023 18:53
marquiz
marquiz reviewed Mar 9, 2023
View reviewed changes
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Thanks @AhmedGrati! Looks good 👍 You just need to update go.mod to include the new dep e.g. by running go mod tidy

@AhmedGrati AhmedGrati force-pushed the feat-implement-e2e-denylabelns-flag branch from 0c80e0b to 7770286 Compare March 9, 2023 19:19
@AhmedGrati AhmedGrati force-pushed the feat-implement-e2e-denylabelns-flag branch from 7770286 to a523505 Compare March 10, 2023 09:40
@AhmedGrati AhmedGrati force-pushed the feat-implement-e2e-denylabelns-flag branch from a523505 to 16abfd7 Compare March 10, 2023 10:11
@AhmedGrati AhmedGrati requested a review from marquiz March 10, 2023 11:32
marquiz
marquiz approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Looks good to me now 👍

ping @fmuyassarov

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AhmedGrati, marquiz

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

fmuyassarov
fmuyassarov reviewed Mar 10, 2023
View reviewed changes
Copy link
Member

@fmuyassarov fmuyassarov left a comment

Choose a reason for hiding this comment

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

/lgtm
Thank you.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 00363c8664a635a92fa447d2f449cf3cbcce48a3

@k8s-ci-robot k8s-ci-robot merged commit 56d186b into kubernetes-sigs:master Mar 10, 2023
@AhmedGrati AhmedGrati mentioned this pull request Apr 11, 2023
Closed
9 tasks
@marquiz marquiz mentioned this pull request Apr 12, 2023
Closed
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmuyassarov fmuyassarov fmuyassarov left review comments

@marquiz marquiz marquiz approved these changes

Assignees

@fmuyassarov fmuyassarov

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: Implement e2e tests of the -deny-label-ns flag
5 participants
@AhmedGrati @k8s-ci-robot @marquiz @fmuyassarov @TessaIO