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

Add scheme to DNS service discovery docs #3450

Merged
merged 2 commits into from
Nov 17, 2022
Merged

Conversation

daquinoaldo
Copy link
Contributor

The missing schema leads to errors like improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?).

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@daquinoaldo daquinoaldo requested review from a team as code owners November 15, 2022 11:33
@CLAassistant
Copy link

CLAassistant commented Nov 15, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR! The issue you raised is interesting. The doc page docs/sources/operators-guide/configure/about-dns-service-discovery.md is a generic one about the DNS-based service discovery. In the example you edited there (memcached one) it's correct to not have a schema (e.g. memcached protocol is not HTTP based). However, when you use the DNS-based service discovery with the alertmanager, you need to set the scheme there.

We can do few things to clarify it:

  1. In docs/sources/operators-guide/configure/about-dns-service-discovery.md, mention the scheme is required when referencing the config option -ruler.alertmanager-url
  2. We can improve the CLI flag description here to clarify it there as well

@daquinoaldo daquinoaldo changed the title Add schema to DNS service discovery docs Add scheme to DNS service discovery docs Nov 15, 2022
@daquinoaldo daquinoaldo requested a review from pracucci November 15, 2022 18:07
@daquinoaldo
Copy link
Contributor Author

Thank you, @pracucci. I've updated the PR.

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM. Could you run make reference-help doc to update the auto-generated doc, please? @osg-grafana can you take a final look at the copy, please?

The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`.
Copy link
Contributor

@osg-grafana osg-grafana left a comment

Choose a reason for hiding this comment

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

Unblocking with feedback

@osg-grafana osg-grafana added the type/docs Improvements or additions to documentation label Nov 16, 2022
@pracucci pracucci enabled auto-merge (squash) November 17, 2022 13:17
@pracucci pracucci merged commit 66df01f into grafana:main Nov 17, 2022
manohar-koukuntla added a commit to manohar-koukuntla/mimir that referenced this pull request Nov 17, 2022
Co-authored-by: Marco Pracucci <[email protected]>

Add scheme to DNS service discovery docs (grafana#3450)

* Add schema to DNS service discovery docs

The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`.

* Update docs/sources/operators-guide/configure/about-dns-service-discovery.md

Co-authored-by: Ursula Kallio <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>
Co-authored-by: Ursula Kallio <[email protected]>

Alertmanager configuration validate-only mode grafana#3437 (grafana#3440)

* Alertmanager configuration validate-only mode grafana#3437

* Documentation update for alertmanager config validation

* Update changelog

* Update docs/sources/operators-guide/architecture/components/alertmanager.md

Co-authored-by: Ursula Kallio <[email protected]>

* Swap a command line flag for a verify subcommand.

* Update pkg/mimirtool/commands/alerts.go

Co-authored-by: Marco Pracucci <[email protected]>

* For loop to reduce repetition

* Use review suggestion.

* Make alertmanager verification docs more consistent.
Bump from enhancement to feature.

* fmt

* Fix the golint issue.

* Fix typo

* Make doc result (as specified by lint process)

Co-authored-by: Ursula Kallio <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>

lint fix
pracucci pushed a commit that referenced this pull request Nov 17, 2022
* add runbook urls for alerts

Co-authored-by: Joshua Hesketh <[email protected]>

Update CHANGELOG.md

Co-authored-by: Joshua Hesketh <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Marco Pracucci <[email protected]>

Add scheme to DNS service discovery docs (#3450)

* Add schema to DNS service discovery docs

The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`.

* Update docs/sources/operators-guide/configure/about-dns-service-discovery.md

Co-authored-by: Ursula Kallio <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>
Co-authored-by: Ursula Kallio <[email protected]>

Alertmanager configuration validate-only mode #3437 (#3440)

* Alertmanager configuration validate-only mode #3437

* Documentation update for alertmanager config validation

* Update changelog

* Update docs/sources/operators-guide/architecture/components/alertmanager.md

Co-authored-by: Ursula Kallio <[email protected]>

* Swap a command line flag for a verify subcommand.

* Update pkg/mimirtool/commands/alerts.go

Co-authored-by: Marco Pracucci <[email protected]>

* For loop to reduce repetition

* Use review suggestion.

* Make alertmanager verification docs more consistent.
Bump from enhancement to feature.

* fmt

* Fix the golint issue.

* Fix typo

* Make doc result (as specified by lint process)

Co-authored-by: Ursula Kallio <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>

lint fix

Co-authored-by: Joshua Hesketh <[email protected]>
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
* Add schema to DNS service discovery docs

The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`.

* Update docs/sources/operators-guide/configure/about-dns-service-discovery.md

Co-authored-by: Ursula Kallio <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>
Co-authored-by: Ursula Kallio <[email protected]>
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
* add runbook urls for alerts

Co-authored-by: Joshua Hesketh <[email protected]>

Update CHANGELOG.md

Co-authored-by: Joshua Hesketh <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Marco Pracucci <[email protected]>

Add scheme to DNS service discovery docs (grafana#3450)

* Add schema to DNS service discovery docs

The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`.

* Update docs/sources/operators-guide/configure/about-dns-service-discovery.md

Co-authored-by: Ursula Kallio <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>
Co-authored-by: Ursula Kallio <[email protected]>

Alertmanager configuration validate-only mode grafana#3437 (grafana#3440)

* Alertmanager configuration validate-only mode grafana#3437

* Documentation update for alertmanager config validation

* Update changelog

* Update docs/sources/operators-guide/architecture/components/alertmanager.md

Co-authored-by: Ursula Kallio <[email protected]>

* Swap a command line flag for a verify subcommand.

* Update pkg/mimirtool/commands/alerts.go

Co-authored-by: Marco Pracucci <[email protected]>

* For loop to reduce repetition

* Use review suggestion.

* Make alertmanager verification docs more consistent.
Bump from enhancement to feature.

* fmt

* Fix the golint issue.

* Fix typo

* Make doc result (as specified by lint process)

Co-authored-by: Ursula Kallio <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>

lint fix

Co-authored-by: Joshua Hesketh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants