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

Drop NFD gRPC API #1910

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Conversation

ArangoGutierrez
Copy link
Contributor

Fixes #1546

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 15, 2024
Copy link

netlify bot commented Oct 15, 2024

Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit 0bd82cf
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/6720ee03d3b7d4000802ec03
😎 Deploy Preview https://deploy-preview-1910--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 configuration.

@ArangoGutierrez
Copy link
Contributor Author

/assign @marquiz

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 15, 2024
@ArangoGutierrez ArangoGutierrez force-pushed the I/1546 branch 2 times, most recently from 69d7a98 to b97027c Compare October 17, 2024 11:15
@ArangoGutierrez
Copy link
Contributor Author

Documentation drop is on PR #1912

@ArangoGutierrez ArangoGutierrez force-pushed the I/1546 branch 3 times, most recently from 5657490 to 7cec6ed Compare October 17, 2024 13:50
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 @ArangoGutierrez for taking this effort. It's a good start. A few comments below.

Also a nit about the commit messages 🙄 Let's specify context in format like docs: xyzzy... instead of bracketted [Documentation]

cmd/nfd-master/main.go Show resolved Hide resolved
pkg/nfd-worker/nfd-worker.go Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be some stale tls leftovers in templates/topologyupdater.yaml as well. Drop those, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding vendor/?? Let's not at least do it in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

meh this was a fat finger, not intended. my bad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2024
@ArangoGutierrez ArangoGutierrez force-pushed the I/1546 branch 2 times, most recently from 7145055 to 5fb7a44 Compare October 24, 2024 09:58
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024
@ArangoGutierrez
Copy link
Contributor Author

Thanks @ArangoGutierrez for taking this effort. It's a good start. A few comments below.

Also a nit about the commit messages 🙄 Let's specify context in format like docs: xyzzy... instead of bracketted [Documentation]

dropped that commit, as I want documentation to be a stand-alone PR

@ArangoGutierrez ArangoGutierrez force-pushed the I/1546 branch 2 times, most recently from ed92a84 to e6ee7ca Compare October 24, 2024 10:04
@ArangoGutierrez
Copy link
Contributor Author

/test pull-node-feature-discovery-e2e-test-master

@ArangoGutierrez
Copy link
Contributor Author

/milestone v0.17

@k8s-ci-robot k8s-ci-robot added this to the v0.17 milestone Oct 24, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2024
@ArangoGutierrez
Copy link
Contributor Author

@marquiz PR is ready for review

}

serverOpts := []grpc.ServerOption{}
tlsConfig := utils.TlsConfig{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the TlsConfig stuff from the utils package, too

cmd/nfd-master/main.go Show resolved Hide resolved
klog.InfoS("-key-file is deprecated, will be removed in a future release along with the deprecated gRPC API")
case "port":
klog.InfoS("-port is deprecated, will be removed in a future release along with the deprecated gRPC API")
case "verify-node-name":
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the -verify-node-name flag from below, too

klog.InfoS("-cert-file is deprecated, will be removed in a future release along with the deprecated gRPC API")
case "key-file":
klog.InfoS("-key-file is deprecated, will be removed in a future release along with the deprecated gRPC API")
case "port":
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the -port flag from below, too

ConfigFile string
Instance string
KeyFile string
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop port and verifynodename, too

pkg/nfd-worker/nfd-worker.go Show resolved Hide resolved
@ArangoGutierrez
Copy link
Contributor Author

/test pull-node-feature-discovery-build-image-cross-generic

2 similar comments
@ArangoGutierrez
Copy link
Contributor Author

/test pull-node-feature-discovery-build-image-cross-generic

@ArangoGutierrez
Copy link
Contributor Author

/test pull-node-feature-discovery-build-image-cross-generic

@@ -303,14 +277,6 @@ func (w *nfdWorker) Run() error {
return err
}

// Create watcher for TLS certificates
w.certWatch, err = utils.CreateFsWatcher(time.Second, w.args.CaFile, w.args.CertFile, w.args.KeyFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's drop certWatch from the nfdWorker struct, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Instance string
Klog map[string]*utils.KlogFlagVal
Kubeconfig string
Port int
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to drop the VerifyNodeName below, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
@ArangoGutierrez
Copy link
Contributor Author

/test pull-node-feature-discovery-build-image-cross-generic

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.

Thank you @ArangoGutierrez for this effort. If/when we missed something let's drop those with follow-up PRs
/lgtm

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

LGTM label has been added.

Git tree hash: fde9597d75a484789afef44f017b681530fc32b5

@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:
  • OWNERS [ArangoGutierrez,marquiz]

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 242410d into kubernetes-sigs:master Oct 30, 2024
10 checks passed
ArangoGutierrez pushed a commit to ArangoGutierrez/node-feature-discovery that referenced this pull request Oct 30, 2024
ArangoGutierrez pushed a commit to ArangoGutierrez/node-feature-discovery that referenced this pull request Nov 4, 2024
@marquiz marquiz mentioned this pull request Dec 12, 2024
24 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop NFD gRPC API
3 participants