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

Fix node-agent missing metrics-addr parms to define the server start. #6784 #6784

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

yanggangtony
Copy link
Contributor

Thank you for contributing to Velero!

Please add a summary of your change

Fix node-agent missing metrics-addr parms to define the server start.

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #6784 (069c280) into main (ad114f8) will decrease coverage by 0.01%.
The diff coverage is 9.09%.

@@            Coverage Diff             @@
##             main    #6784      +/-   ##
==========================================
- Coverage   60.78%   60.78%   -0.01%     
==========================================
  Files         250      250              
  Lines       26629    26632       +3     
==========================================
+ Hits        16187    16188       +1     
- Misses       9293     9295       +2     
  Partials     1149     1149              
Files Coverage Δ
pkg/install/daemonset.go 100.00% <100.00%> (ø)
pkg/cmd/cli/nodeagent/server.go 8.86% <0.00%> (-0.06%) ⬇️

@allenxu404
Copy link
Contributor

Taking a quick look at this PR, I think the daemonset of node agent should also expose the container port same as how Velero deployment works:
https://github.com/vmware-tanzu/velero/blob/8a366c6924a756e70cd352a6063aaaf38ec30e9f/pkg/install/deployment.go#L217C42-L217C42

@yanggangtony
Copy link
Contributor Author

@allenxu404

Taking a quick look at this PR, I think the daemonset of node agent should also expose the container port same as how Velero deployment works:

Good catch .
Would you suggest arise another pr for fix that?
Or just in this one?

@yanggangtony yanggangtony force-pushed the node-agent-metrics-addr branch from 84605ae to 74e783b Compare September 12, 2023 11:02
@yanggangtony
Copy link
Contributor Author

I pushd again.
Because this is related to the metrics port..

@allenxu404
Copy link
Contributor

@yanggangtony Can you help repush the PR to trigger CI again since the checks didn't pass on the last run?

@yanggangtony yanggangtony force-pushed the node-agent-metrics-addr branch from 74e783b to 069c280 Compare October 13, 2023 02:33
@yanggangtony
Copy link
Contributor Author

Yep , rebased.

@yanggangtony
Copy link
Contributor Author

Now the codecov shows code diff issues.
I see too many this errors in others prs.😀😀

@blackpiglet blackpiglet merged commit 9606df6 into vmware-tanzu:main Oct 13, 2023
22 of 24 checks passed
@yanggangtony yanggangtony deleted the node-agent-metrics-addr branch October 13, 2023 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants