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

nfd-worker: add core.featureSources config option #605

Merged

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Sep 21, 2021

  • Add a configuration option for controlling the enabled "raw" feature
    sources. This is useful e.g. in testing and development, plus it also
    allows fully shutting down discovery of features that are not needed in
    a deployment. Supplements core.labelSources which controls the
    enablement of label sources.
  • add -feature-sources command line flag
    Allows controlling (enable/disable) the "raw" feature detection.
    Especially useful for development and testing.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 21, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 21, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Sep 21, 2021

Makes more sense (and can be tested) when #604 has been merged.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2021
@marquiz marquiz force-pushed the devel/feature-source-config-flag branch from c36f86e to 83aa356 Compare November 12, 2021 07:42
@marquiz
Copy link
Contributor Author

marquiz commented Nov 12, 2021

Rebased
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2021
@marquiz marquiz force-pushed the devel/feature-source-config-flag branch from 83aa356 to 626d5d1 Compare November 24, 2021 10:24
@marquiz
Copy link
Contributor Author

marquiz commented Nov 25, 2021

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2021
@marquiz marquiz force-pushed the devel/feature-source-config-flag branch from 626d5d1 to a76b8bc Compare November 29, 2021 13:38
@marquiz marquiz changed the title nfd-worker: add core.rawFeatureSources config option nfd-worker: add core.featureSources config option Nov 29, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Nov 29, 2021

Renamed to core.featureSources. Now depends on #603.

@ArangoGutierrez
Copy link
Contributor

/test all

@ArangoGutierrez
Copy link
Contributor

no need for rebase, hooray!
reviewing
/assign

@ArangoGutierrez
Copy link
Contributor

Renamed to core.featureSources. Now depends on #603.

#603 is now merged, we are safe to unhold

Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

Why is commit from #671 here?

@marquiz
Copy link
Contributor Author

marquiz commented Nov 29, 2021

Why is commit from #671 here?

It's a bit of a mess. Rebased on top of #673

Dependency chain #671 -> #673 -> #605

@ArangoGutierrez
Copy link
Contributor

Why is commit from #671 here?

It's a bit of a mess. Rebased on top of #673

Dependency chain #671 -> #673 -> #605

#671 and #673 are in, we are ready for this one

/test all

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021
@ArangoGutierrez
Copy link
Contributor

@marquiz rebase 😁

@marquiz marquiz force-pushed the devel/feature-source-config-flag branch from a76b8bc to 36ad649 Compare December 2, 2021 09:04
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Dec 2, 2021

@marquiz rebase

Rebased. Plus I fixed some mistakes that I spotted myself
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Dec 2, 2021

/assign zvonkok

@marquiz
Copy link
Contributor Author

marquiz commented Dec 2, 2021

Need to rebase and align this with #670

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2021
Refactoring in head of adding new config option for feature sources.
@marquiz marquiz force-pushed the devel/feature-source-config-flag branch from 36ad649 to a8d57bb Compare December 2, 2021 19:23
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Dec 2, 2021

Rebased, I haven't verified this new rebase myself, though

@marquiz
Copy link
Contributor Author

marquiz commented Dec 3, 2021

Rebased, I haven't verified this new rebase myself, though

Verified that it works as expected

@marquiz marquiz force-pushed the devel/feature-source-config-flag branch from a8d57bb to 02f0364 Compare December 3, 2021 07:24
@marquiz
Copy link
Contributor Author

marquiz commented Dec 3, 2021

Amended this PR with a new patch adding -feature-sources cmdline flag. Makes this analogous to core.labelSources vs. -label-sources

Add a configuration option for controlling the enabled "raw" feature
sources. This is useful e.g. in testing and development, plus it also
allows fully shutting down discovery of features that are not needed in
a deployment. Supplements core.labelSources which controls the
enablement of label sources.
Allows controlling (enable/disable) the "raw" feature detection.
Especially useful for development and testing.
Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, 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

@k8s-ci-robot k8s-ci-robot merged commit 6071b04 into kubernetes-sigs:master Dec 3, 2021
@marquiz marquiz deleted the devel/feature-source-config-flag branch December 7, 2021 08:12
@marquiz marquiz mentioned this pull request Dec 22, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants