Skip to content

cmd/helm-operator/main.go: add --metrics-addr flag#3440

Merged
joelanford merged 4 commits intooperator-framework:masterfrom
joelanford:helm-metrics-addr
Jul 16, 2020
Merged

cmd/helm-operator/main.go: add --metrics-addr flag#3440
joelanford merged 4 commits intooperator-framework:masterfrom
joelanford:helm-metrics-addr

Conversation

@joelanford
Copy link
Copy Markdown
Member

Description of the change:
Add --metrics-addr flag for Helm operator

Motivation for the change:
#2451 #3358

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@joelanford joelanford added this to the v1.0.0 milestone Jul 16, 2020
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Just a nit in the fragment. Otherwise, it shows great
/lgtm

migration:
header: Default helm operator metrics port changed
body: >
To continue using port 8383, specify `--metrics-addr=:8383` when you start the operator.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

WDYT about we improve it and describe where the --metrics-addr=:8383 need to be used (config/default, and config/manager/)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I disagree for a few reasons:

  1. In general, it should be obvious to most operator authors, and kubernetes users how to set flags on their operator.
  2. In fairness, it is somewhat confusing when kustomize enters the picture, so I would propose that we write a general section in a doc somewhere about how to set flags on the manager when using the kustomize templates. This should probably be a kubebuidler doc.
  3. There are lots of flags on operators. We shouldn't have to repeat ourselves in migration guides, docs, etc. anytime we discuss setting flags.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Far enough.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020
@openshift-ci-robot
Copy link
Copy Markdown

New changes are detected. LGTM label has been removed.

@joelanford joelanford closed this Jul 16, 2020
@joelanford joelanford reopened this Jul 16, 2020
@joelanford joelanford closed this Jul 16, 2020
@joelanford joelanford reopened this Jul 16, 2020
Comment on lines -90 to -91
- args:
- manager
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This removal is no directly related to this PR, but needs to happen to avoid problems running the operator in a cluster.

This is an accidental carryover from Go operators that have a manager binary. But helm-operator has a different binary entrypoint: the base image adds the binary and sets the entrypoint for us, so we just need to specify the flags to pass to the helm-operator binary.

@joelanford joelanford merged commit e9865b0 into operator-framework:master Jul 16, 2020
@joelanford joelanford deleted the helm-metrics-addr branch July 16, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants