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

[stable/prometheus-operator] Add conditional for prometheus.ingress.hosts #16977

Merged
merged 3 commits into from Sep 9, 2019

Conversation

@phyrog
Copy link
Contributor

commented Sep 9, 2019

Is this a new chart

No

What this PR does / why we need it:

This PR checks for the existence of .Values.prometheus.ingress.hosts in addition to .Values.prometheus.ingress.enabled before setting the externalUrl field.

As there is an explicit check for .Values.prometheus.ingress.hosts here, the value does not seem to be required, but not setting the field causes an error at the changed location.

Which issue this PR fixes

None that I am aware of

Special notes for your reviewer:

None

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [stable/chart])
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

Hi @phyrog. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@phyrog

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

/assign @gianrubio

@vsliouniaev

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

@phyrog Mind adding this for alertmanager as well?

phyrog added 3 commits Sep 9, 2019
Add conditional for prometheus.ingress.hosts
Signed-off-by: Tom Gehrke <t.gehrke@reply.de>
Bump patch version to 6.9.1
Signed-off-by: Tom Gehrke <t.gehrke@reply.de>
Add condition for alertmanager as well
Signed-off-by: Tom Gehrke <t.gehrke@reply.de>
@phyrog

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

@vsliouniaev done. I also rebased on master due to a version bump by a different PR and subsequently bumped the patch version based on the new version.

@vsliouniaev

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

/ok-to-test
/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phyrog, vsliouniaev

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 c6df9a9 into helm:master Sep 9, 2019

6 checks passed

DCO DCO
Details
ci/circleci: lint-charts Your tests passed on CircleCI!
Details
ci/circleci: lint-scripts Your tests passed on CircleCI!
Details
dco-labeler All commits have signoff
pull-charts-e2e Job succeeded.
Details
tide In merge pool.
Details
jtnz added a commit to jtnz/charts that referenced this pull request Sep 10, 2019
[stable/prometheus-operator] Add conditional for prometheus.ingress.h…
…osts (helm#16977)

* Add conditional for prometheus.ingress.hosts

Signed-off-by: Tom Gehrke <t.gehrke@reply.de>

* Bump patch version to 6.9.1

Signed-off-by: Tom Gehrke <t.gehrke@reply.de>

* Add condition for alertmanager as well

Signed-off-by: Tom Gehrke <t.gehrke@reply.de>

@phyrog phyrog deleted the phyrog:patch-1 branch Sep 10, 2019

mariusv added a commit to mariusv/charts that referenced this pull request Sep 16, 2019
[stable/prometheus-operator] Add conditional for prometheus.ingress.h…
…osts (helm#16977)

* Add conditional for prometheus.ingress.hosts

Signed-off-by: Tom Gehrke <t.gehrke@reply.de>

* Bump patch version to 6.9.1

Signed-off-by: Tom Gehrke <t.gehrke@reply.de>

* Add condition for alertmanager as well

Signed-off-by: Tom Gehrke <t.gehrke@reply.de>
Signed-off-by: Marius Voila <myself@mariusv.com>
kengou added a commit to kengou/charts that referenced this pull request Sep 18, 2019
[stable/prometheus-operator] Add conditional for prometheus.ingress.h…
…osts (helm#16977)

* Add conditional for prometheus.ingress.hosts

Signed-off-by: Tom Gehrke <t.gehrke@reply.de>

* Bump patch version to 6.9.1

Signed-off-by: Tom Gehrke <t.gehrke@reply.de>

* Add condition for alertmanager as well

Signed-off-by: Tom Gehrke <t.gehrke@reply.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.