-
Notifications
You must be signed in to change notification settings - Fork 810
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
Enable compactor and alertmanager in target all #6204
Enable compactor and alertmanager in target all #6204
Conversation
9cab6ca
to
8c62379
Compare
@friedrichg |
8c62379
to
6913d2b
Compare
@friedrichg |
pkg/cortex/cortex_test.go
Outdated
} | ||
|
||
externalURL := flagext.URLValue{} | ||
err := externalURL.Set("http://localhost/api/prom") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err := externalURL.Set("http://localhost/api/prom") | |
err := externalURL.Set("http://localhost/alertmanager") |
it's more appropriate
I think you can fix most integration tests adding the following snippet to the yamls in docs/configuration
or something similar |
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
6913d2b
to
fd5ee03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't do what I suggested, but that is ok. You solved the problem.
The getting-started is not used in the docs anymore, so more work is needed there. Not to be done in this PR, for sure.
@friedrichg |
@CharlieTLe |
I think this change breaks something but I am not sure why it is not captured by tests. For example, it failed to run Cortex with provided example config file https://github.com/cortexproject/cortex/blob/master/docs/configuration/single-process-config-blocks-local.yaml because the config file misses alertmanager storage configurations |
@yeya24
|
I see. @SungJin1212 Can we please fix the config file as well? |
@yeya24 |
What this PR does:
Enable compactor and alertmanager in target all.
Which issue(s) this PR fixes:
Fixes #6098
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]