Skip to content

Comments

chore: add alert package, deprecate alert in types#5043

Merged
siavashs merged 2 commits intoprometheus:mainfrom
siavashs:chore/alert-package
Feb 24, 2026
Merged

chore: add alert package, deprecate alert in types#5043
siavashs merged 2 commits intoprometheus:mainfrom
siavashs:chore/alert-package

Conversation

@siavashs
Copy link
Contributor

@siavashs siavashs commented Feb 23, 2026

The types package is a well-known Go anti-pattern.
This is a change from a series that split the types package.

This change adds a new alert package which only includes alert types.
The same types are re-exported from types for backward compatibility,
and marked as deprecated.

GolangCI static checks are disabled for types.Alert* until we are ready for refactoring the code.

Pull Request Checklist

Please check all the applicable boxes.

  • Please list all open issue(s) discussed with maintainers related to this change
    • Fixes #
  • Is this a breaking change?
    • My changes do not break the existing api
  • I have signed-off my commits
  • I will follow best practices for contributing to this project

Which user-facing changes does this PR introduce?

[CHANGE] Deprecate types.Alert*, add alert package

The `types` package is a well-known Go anti-pattern.
This is a change from a series that split the `types` package.

This change adds a new `alert` package which only includes alert types.
The same types are re-exported from `types` for backward compatibility,
and marked as deprecated.

Signed-off-by: Siavash Safi <siavash@cloudflare.com>
Copy link
Contributor

@SoloJacobs SoloJacobs left a comment

Choose a reason for hiding this comment

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

In principal a nice change, but I do think it is user facing? Won't users have linting errors, if we deprecate things?

Signed-off-by: Siavash Safi <siavash@cloudflare.com>
@siavashs
Copy link
Contributor Author

In principal a nice change, but I do think it is user facing? Won't users have linting errors, if we deprecate things?

Marking public types and methods in Go is the only way to let downstream package users know that they should migrate.
They will have the option to ignore the deprecation errors for now or migrate to the new alert package.

@SoloJacobs
Copy link
Contributor

Marking public types and methods in Go is the only way to let downstream package users know that they should migrate. They will have the option to ignore the deprecation errors for now or migrate to the new alert package.

I'm not sure how this related to my review. Let me try and put it differently: At our company there would be a change in the change-log for the deprecation, and another entry if the function is actually removed. Here you are saying user-facing changes are NONE.

@ultrotter
Copy link
Contributor

I think the two options are deprecating as you do, or creating a "v2" version of the library, and leave the current one as is... But that's much more cumbersome to us as for every change we may need to keep deciding how v1 will behave, how v2 will, etc... :/ And hey, we are version 0.X so we never promised "infinite forward compatibility"

@ultrotter
Copy link
Contributor

I do agree we need to mention it in the changelog, to be clear. Not that we shouldn't do it at all ever....

@siavashs
Copy link
Contributor Author

siavashs commented Feb 23, 2026

Marking public types and methods in Go is the only way to let downstream package users know that they should migrate. They will have the option to ignore the deprecation errors for now or migrate to the new alert package.

I'm not sure how this related to my review. Let me try and put it differently: At our company there would be a change in the change-log for the deprecation, and another entry if the function is actually removed. Here you are saying user-facing changes are NONE.

Ah, NONE here means no change for a user using Alertmanager Application, we can expand the definition of "user".
It is Ok to mark this as a DEPRECATION of Go types in changelog, but have we done this ever in Alertmanager?
(Also note that since we do not have these packages under pkg/ then changing the packages should be fine since they are not meant to be used externally.)

So to summarise: it is fine with me if we want to exapnd the definition of "user" to anyone that might pull Alertmanager packages and mark this as DEPRECATION.

@siavashs siavashs requested a review from SoloJacobs February 23, 2026 16:50
@siavashs siavashs mentioned this pull request Feb 23, 2026
2 tasks
@SoloJacobs
Copy link
Contributor

I never meant that we should not deprecate and remove. Users can migrate to the new version without issue. So, adjusting the text seems good me, and is consistent with other changes we have done.

@siavashs siavashs merged commit 94ee49f into prometheus:main Feb 24, 2026
6 checks passed
@siavashs siavashs deleted the chore/alert-package branch February 24, 2026 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants