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

feat: add summary to msteams #6206

Merged
merged 4 commits into from Mar 22, 2024

Conversation

heliapb
Copy link
Contributor

@heliapb heliapb commented Jan 5, 2024

Description

Fix issue #6131

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.


@heliapb heliapb requested a review from a team as a code owner January 5, 2024 17:37
@ArthurSens
Copy link
Member

Looks like we'll need until Alertmanager 0.27 for this one

Copy link
Contributor

@nicolastakashi nicolastakashi left a comment

Choose a reason for hiding this comment

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

Hey we need also add this new field to other places such as:

I think worth also add some unit tests for this new field just to ensure we are not missing any other place on the config, you can find the unit tests available here:

Copy link
Contributor

@dongjiang1989 dongjiang1989 left a comment

Choose a reason for hiding this comment

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

ref: #6365

pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Outdated Show resolved Hide resolved
pkg/apis/monitoring/v1beta1/alertmanager_config_types.go Outdated Show resolved Hide resolved
Comment on lines 1627 to 1629
if config.Summary != nil && amVersion.LT(semver.MustParse("0.27.0")) {
return fmt.Errorf(`Summary only available in Alertmanager >= 0.27.0`)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

move to func (tc *msTeamsConfig) sanitize(amVersion semver.Version, logger log.Logger) error {...}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @dongjiang1989 changed accordingly, could you review?

@heliapb heliapb changed the title WIP - feat: add summary to msteams feat: add summary to msteams Mar 19, 2024
@heliapb
Copy link
Contributor Author

heliapb commented Mar 19, 2024

@slashpai could you pls review?

Copy link
Contributor

@slashpai slashpai left a comment

Choose a reason for hiding this comment

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

lgtm, just a nit

pkg/alertmanager/amcfg.go Outdated Show resolved Hide resolved
@heliapb heliapb requested a review from slashpai March 19, 2024 17:58
@slashpai
Copy link
Contributor

@nicolastakashi Take a look if changes are good. IIUC merging is blocked since requested changes and you would need to approve to proceed :)

@nicolastakashi nicolastakashi merged commit e016b5b into prometheus-operator:main Mar 22, 2024
17 checks passed
@heliapb heliapb deleted the feat/mstsummary branch March 22, 2024 12:16
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.

None yet

5 participants