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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 fix: preserve existing flags when applying metrics patch #3937

Merged
merged 1 commit into from
May 22, 2024
Merged

馃悰 fix: preserve existing flags when applying metrics patch #3937

merged 1 commit into from
May 22, 2024

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented May 20, 2024 •
edited
Loading

Ensure that enabling the manager_metrics_patch.yaml in config/default/kustomization.yaml does not overwrite existing arguments in config/manager/manager.yaml. The patch now appends the --metrics-bind-address argument without replacing other arguments.

Closes: #3934

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 20, 2024
@camilamacedo86 camilamacedo86 mentioned this pull request May 20, 2024
Closed
This was referenced May 20, 2024
Closed
Closed
@joelanford
Copy link
Member

This solves it for the default set of flags that we scaffold into config/manager/manager.yaml, but if someone wants to add a new flag they still have to add it to both places. That's why I'm wondering if we could use json 6902 patch style so that the patch file inserts an argument into the list provided by the base.

joelanford
joelanford reviewed May 21, 2024
View reviewed changes
...ins/common/kustomize/v2/scaffolds/internal/templates/config/kdefault/enable_matrics_patch.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

By the way, typo in file name:

s/enable_matrics_patch.go/enable_metrics_patch.go/

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw this one as well. I was avoiding have issues to cherry-pick but lets fix it.
The correct way is keep the go file name as the name of the file that is scaffolded.
Done 馃憤

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 21, 2024
@camilamacedo86
Copy link
Member Author

Hi @joelanford

Regards: #3937 (comment)
I changed it. We can do as you suggested but still we need alert about the position since it is based in positions.
But I think it might be better as you suggested.

@camilamacedo86 camilamacedo86 mentioned this pull request May 21, 2024
Closed
@camilamacedo86 camilamacedo86 changed the title 馃悰 fix: preserve existing flags when applying metrics patch (WIP)馃悰 fix: preserve existing flags when applying metrics patch May 21, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2024
@camilamacedo86 camilamacedo86 changed the title (WIP)馃悰 fix: preserve existing flags when applying metrics patch (WIP - not passing in the e2e tests after the changes / required to check) 馃悰 fix: preserve existing flags when applying metrics patch May 21, 2024
@camilamacedo86 camilamacedo86 changed the title (WIP - not passing in the e2e tests after the changes / required to check) 馃悰 fix: preserve existing flags when applying metrics patch 馃悰 fix: preserve existing flags when applying metrics patch May 21, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2024
@camilamacedo86 camilamacedo86 force-pushed the fix-patch-metrics branch 2 times, most recently from 933fd41 to 6c3321b Compare May 22, 2024 05:11
@camilamacedo86 camilamacedo86 mentioned this pull request May 22, 2024
Merged
joelanford
joelanford reviewed May 22, 2024
View reviewed changes
docs/book/src/cronjob-tutorial/testdata/project/cmd/main.go Outdated
@@ -76,7 +76,8 @@ func main() {
var probeAddr string
var secureMetrics bool
var enableHTTP2 bool
flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
flag.StringVar(&metricsAddr, "metrics-bind-address", ":0", "The address the metric endpoint binds to. "+
Copy link
Member

Choose a reason for hiding this comment

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

I think ":0" means "bind to any open port", whereas "0" means "don't start the metrics server", right?

Suggested change
flag.StringVar(&metricsAddr, "metrics-bind-address", ":0", "The address the metric endpoint binds to. "+
flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metric endpoint binds to. "+

Copy link
Member Author

Choose a reason for hiding this comment

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

That works as well. But we can change no problem at all.

joelanford
joelanford reviewed May 22, 2024
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

One more fix, I think. Other than that, lgtm.

@camilamacedo86
fix: preserve existing flags when applying metrics patch
d05aae5 
Ensure that enabling the manager_metrics_patch.yaml in config/default/kustomization.yaml does not overwrite existing arguments in config/manager/manager.yaml. The patch now appends the --metrics-bind-address argument without replacing other arguments.

More info: kubernetes-sigs#3934
@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2024
@k8s-ci-robot k8s-ci-robot merged commit 16d5cef into kubernetes-sigs:master May 22, 2024
18 checks passed
k8s-ci-robot added a commit that referenced this pull request May 23, 2024
@k8s-ci-robot
Merge pull request #3947 from camilamacedo86/patch-release
445eb2a 
馃悰 (cherry-pick #3937) - fix: preserve existing flags when applying metrics patch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford left review comments

@everettraven everettraven Awaiting requested review from everettraven

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

Assignees
No one assigned
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. release-blocker 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.

kustomize: strategicMergePatch clobbers patched arrays
3 participants
@camilamacedo86 @k8s-ci-robot @joelanford