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.

Sign up for GitHub

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

Jump to bottom

feat: support user_key_file and token_file to the pushover #5886

Merged
merged 21 commits into from Oct 6, 2023
Merged

feat: support user_key_file and token_file to the pushover #5886

merged 21 commits into from Oct 6, 2023

Conversation

dongjiang1989
Copy link
Contributor

@dongjiang1989 dongjiang1989 commented Sep 4, 2023 •
edited

…config.

Description

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.

Add the user_key_file and token_file keys to the pushover config.

fix: #5859

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

Add the user_key_file and token_file keys to the pushover config.

@dongjiang1989
dongjiang, add the user_key_file and token_file keys to the pushover …
5720840 
…config.

Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
@dongjiang1989 dongjiang1989 requested a review from a team as a code owner September 4, 2023 15:00
@dongjiang1989
add PushoverConfig Sanitize
133e828 
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 4, 2023
@dongjiang1989
add PushoverConfig Sanitize
4344770 
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
@dongjiang1989 dongjiang1989 mentioned this pull request Sep 4, 2023
Closed
2 tasks
simonpasquier
simonpasquier reviewed Sep 4, 2023
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

Thanks! This is a very good start. You need also to implement the conversions in pkg/apis/monitoring/v1beta1/conversion_from.go and pkg/apis/monitoring/v1beta1/conversion_to.go. And validations in pkg/alertmanager/validation/*/validation.go.

pkg/alertmanager/amcfg.go Outdated Show resolved Hide resolved
pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Outdated Show resolved Hide resolved
pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Outdated Show resolved Hide resolved
pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Outdated Show resolved Hide resolved
pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Outdated Show resolved Hide resolved
pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Show resolved Hide resolved
pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Show resolved Hide resolved
pkg/apis/monitoring/v1beta1/alertmanager_config_types.go Outdated Show resolved Hide resolved
@dongjiang1989
dongjiang, update by code review
3755399 
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
@dongjiang1989
Copy link
Contributor Author

Thanks! This is a very good start. You need also to implement the conversions in pkg/apis/monitoring/v1beta1/conversion_from.go and pkg/apis/monitoring/v1beta1/conversion_to.go. And validations in pkg/alertmanager/validation/*/validation.go.

Thanks for you review. @simonpasquier Please re-check!

@dongjiang1989
fix unittest case
3dbd810 
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
@dongjiang1989
Merge branch 'main' into add-key-files
4e102d1
@dongjiang1989
update unittest case
20b1cda 
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
ArthurSens
ArthurSens reviewed Sep 15, 2023
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Good work so far!

pkg/alertmanager/amcfg.go Show resolved Hide resolved
pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
pkg/alertmanager/amcfg_test.go Show resolved Hide resolved
pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
pkg/alertmanager/validation/v1beta1/validation.go Outdated Show resolved Hide resolved
pkg/alertmanager/validation/v1beta1/validation.go Outdated Show resolved Hide resolved
pkg/alertmanager/validation/v1beta1/validation_test.go Outdated Show resolved Hide resolved
dongjiang1989 and others added 4 commits September 18, 2023 10:45
@dongjiang1989 @ArthurSens
Update pkg/alertmanager/amcfg_test.go
20a0506 
fix misspell

Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
@dongjiang1989 @ArthurSens
Update pkg/alertmanager/validation/v1beta1/validation_test.go
2af73eb 
fix misspell

Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
@dongjiang1989
update code review case
885b726 
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
@dongjiang1989
add tabale unittest
f695d68 
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
@dongjiang1989
Copy link
Contributor Author

Good work so far!

@ArthurSens thanks for your review.
Please re-check.

@heliochronix
Copy link

Is it possible this will be merged before the next release?

ArthurSens
ArthurSens approved these changes Oct 4, 2023
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens 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 continuing working on this! We're almost there, just some test descriptions that aren't clear and a test that could increase coverage if we split them in two

pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
dongjiang1989 and others added 7 commits October 6, 2023 20:33
@dongjiang1989
Merge branch 'main' into add-key-files
decbd77
@dongjiang1989 @ArthurSens
Update pkg/alertmanager/amcfg_test.go
3dd477b 
Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
@dongjiang1989 @ArthurSens
Update pkg/alertmanager/amcfg_test.go
6e0db6e 
Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
@dongjiang1989 @ArthurSens
Update pkg/alertmanager/amcfg_test.go
f20eac8 
Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
@dongjiang1989 @ArthurSens
Update pkg/alertmanager/amcfg_test.go
c21388f 
Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
@dongjiang1989 @ArthurSens
Update pkg/alertmanager/amcfg_test.go
3aec801 
Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
@dongjiang1989
add unittest case by code review
390494e 
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
ArthurSens
ArthurSens approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

I'm so sorry for the bumpy interactions, I think I missed these in the previous review 😭

pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
pkg/alertmanager/amcfg_test.go Outdated Show resolved Hide resolved
dongjiang1989 and others added 2 commits October 6, 2023 23:41
@dongjiang1989 @ArthurSens
Update pkg/alertmanager/amcfg_test.go
edd0cc6 
Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
@dongjiang1989 @ArthurSens
Update pkg/alertmanager/amcfg_test.go
0d25c3c 
Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
@dongjiang1989
Copy link
Contributor Author

I'm so sorry for the bumpy interactions, I think I missed these in the previous review 😭

@ArthurSens Thank you for your review.

@dongjiang1989
fix golint
8714125 
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
ArthurSens
ArthurSens approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Thank you very much!!

@ArthurSens ArthurSens merged commit e3798b0 into prometheus-operator:main Oct 6, 2023
17 checks passed
@dongjiang1989 dongjiang1989 deleted the add-key-files branch October 19, 2023 07:17
@slashpai slashpai mentioned this pull request Mar 5, 2024
Open
@slashpai slashpai mentioned this pull request Mar 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonpasquier simonpasquier simonpasquier left review comments

@ArthurSens ArthurSens ArthurSens approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support device, user_key_file and token_file for Pushover integration
4 participants
@dongjiang1989 @heliochronix @simonpasquier @ArthurSens