kubernetes-sigs
feat/nfd-master: configure CR restrictions
Resolves #1380 Features implemented:
- NodeFeatures Namespace selection by using selectors.
- Max number of Taints, Labels, and ERs that can be generated by a single CR.
- Enable/Disable overriding labels.
- Enable/Disable NodeFeatures labels.
Result of e2e tests:
Welcome @AhmedThresh!
It looks like this is your first PR to kubernetes-sigs/node-feature-discovery ๐. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes-sigs/node-feature-discovery has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @AhmedThresh. 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.
Deploy Preview for kubernetes-sigs-nfd ready!
| Name | Link |
|---|---|
| Latest commit | 7fa344fcf0ead9351dfc1f25d881164efff3ee8a |
| Latest deploy log | https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/662e184758e83b0008706b09 |
| Deploy Preview | https://deploy-preview-1592--kubernetes-sigs-nfd.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: TessaIO (6e464a691122f0537076b2a55e17850e6044d55a, 7fa344fcf0ead9351dfc1f25d881164efff3ee8a, 7b1e751cf1f699431ea607d22e86cb205462d0c6)
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: AhmedThresh Once this PR has been reviewed and has the lgtm label, please ask for approval from marquiz. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Thanks for the review @marquiz and @ArangoGutierrez. I'll continue working on this ASAP.
ping @TessaIO, do you think you have time to work on this for v0.16? No pressure, we can defer to v0.17, just asking ๐
Ummm @TessaIO I think once this PR is ready for merge, we should close it, and you should re open it under TessaIO to add more contributions to TessaIO and not merge this temporal Account AhmedThressh
One nit comment (besides the pages tests failing....) is that we have 3 commits for 4 major changes
- NodeFeatures Namespace selection by using selectors.
- Max number of Taints, Labels, and ERs that can be generated by a single CR.
- Enable/Disable overriding labels.
- Enable/Disable NodeFeatures labels.
it would be good to accommodate it, so each commit is self contained for those 4 changes you describe
One nit comment (besides the pages tests failing....) is that we have 3 commits for 4 major changes
- NodeFeatures Namespace selection by using selectors.
- Max number of Taints, Labels, and ERs that can be generated by a single CR.
- Enable/Disable overriding labels.
- Enable/Disable NodeFeatures labels.
it would be good to accommodate it, so each commit is self contained for those 4 changes you describe
Thanks @ArangoGutierrez. That actually depends on how you view the feature. from my POV, I see it as implementation -> deployment -> documentation. But I don't mind creating 4 commits each for a separate feature instead. Also, I couldn't find the reason for pages test failure. Netlify didn't show anything. Any clue about that?
@ArangoGutierrez @marquiz can you spot what's wrong here? I can build on my local machine and golangci-lint works without any error
@TessaIO ha, it tests the "merge head" i.e. PR merged into master. Try rebase and you see the problem
Codecov Report
Attention: Patch coverage is 34.93976% with 54 lines in your changes are missing coverage. Please review.
Project coverage is 39.56%. Comparing base (
bb36c21) to head (834207c).
:exclamation: Current head 834207c differs from pull request most recent head 7fa344f. Consider uploading reports for the commit 7fa344f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## master #1592 +/- ##
==========================================
- Coverage 39.71% 39.56% -0.16%
==========================================
Files 80 80
Lines 6841 6898 +57
==========================================
+ Hits 2717 2729 +12
- Misses 3864 3906 +42
- Partials 260 263 +3
| Files | Coverage ฮ | |
|---|---|---|
| pkg/nfd-master/nfd-master.go | 41.48% <48.71%> (+0.46%) |
:arrow_up: |
| pkg/nfd-master/nfd-api-controller.go | 17.50% <22.72%> (+4.55%) |
:arrow_up: |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@AhmedThresh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-node-feature-discovery-build-image-generic | 7fa344fcf0ead9351dfc1f25d881164efff3ee8a | link | true | /test pull-node-feature-discovery-build-image-generic |
| pull-node-feature-discovery-build-image-cross-generic | 7fa344fcf0ead9351dfc1f25d881164efff3ee8a | link | true | /test pull-node-feature-discovery-build-image-cross-generic |
| pull-node-feature-discovery-build-master | 7fa344fcf0ead9351dfc1f25d881164efff3ee8a | link | true | /test pull-node-feature-discovery-build-master |
| pull-node-feature-discovery-verify-master | 7fa344fcf0ead9351dfc1f25d881164efff3ee8a | link | true | /test pull-node-feature-discovery-verify-master |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
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. I understand the commands that are listed here.
@marquiz rebased but still same error
Yeah, you need to fix the build failure ๐ The function prototype(s)/args have changed a bit so you need to adjust the impl in the patch
The problem is that the build command works on my workstation
I don't know what you're testing locally but the version in this PR cannot be built
EDIT: looking at the git sha in your screenshot you're building some very old version of the PR
@AhmedThresh / @TessaIO Looks like the current version of this PR doesn't build / pass the basic CI