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

VictorOps api-key <SECRET> via file feature #2554

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

gr8Adakron
Copy link

Adding features for inserting VictorOps-api-key secret through file - as suggested in this issue. I haven't tested in local not much familiar with GO, working on that. Any high level review will be really helpful - if I am going in right direction or not. Thanks

@gr8Adakron
Copy link
Author

gr8Adakron commented Apr 24, 2021

@beorn7 could you please help out here a bit, not sure why getting so many test case failure. I followed the same structure as that slack_api_url_file commits.

Not much familiarity with GO

@beorn7
Copy link
Member

beorn7 commented Apr 27, 2021

The first thing to fix is easy: Add the DCO. See https://github.com/prometheus/alertmanager/pull/2554/checks?check_run_id=2426690259 . No Go knowledge required. (o:

@beorn7
Copy link
Member

beorn7 commented Apr 27, 2021

You have partially reverted recent changes that added support to read the Slack URL from a file. That's also the reason why the tests and the build fail.

Perhaps you have branched of from the master branch before that feature was merged, and then you did not resolve the merge or rebase properly? I'll add a few comments where this happened.

Comment on lines +304 to +306
if c.Global.SlackAPIURL != nil {
return fmt.Errorf("at most one of slack_api_url must be configured")
}
Copy link
Member

Choose a reason for hiding this comment

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

This code is about Slack and should not change.

SMTPAuthIdentity string `yaml:"smtp_auth_identity,omitempty" json:"smtp_auth_identity,omitempty"`
SMTPRequireTLS bool `yaml:"smtp_require_tls" json:"smtp_require_tls,omitempty"`
SlackAPIURL *SecretURL `yaml:"slack_api_url,omitempty" json:"slack_api_url,omitempty"`
SlackAPIURLFile string `yaml:"slack_api_url_file,omitempty" json:"slack_api_url_file,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Here you are accidentally removing this line about Slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants