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

jira integration #3590

Merged
merged 46 commits into from
Jul 31, 2024
Merged

jira integration #3590

merged 46 commits into from
Jul 31, 2024

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Nov 1, 2023

This PR track the Atlassian JIRA integration into Alertmanger.

See: #3577

Replaces: https://github.com/prometheus-community/jiralert


What I test:

  • Jira Cloud only
  • Create issues
  • Auto-Close issue with resolve_transition
  • Reopen issue with reopen_transition
  • Reopen duration
  • Custom Fields
  • api_token_file

@jkroepke jkroepke changed the title WIP: jira integration jira integration Nov 1, 2023
@jkroepke jkroepke marked this pull request as ready for review November 1, 2023 20:32
Signed-off-by: Jan-Otto Kröpke <[email protected]>
@k0ste
Copy link

k0ste commented Nov 18, 2023

Can this integration cover another of jiralert issue - is custom field for alert hash?
prometheus-community/jiralert#164

Current code search hash in hardcoded place - in labels

jql.WriteString(fmt.Sprintf(`project = "%s" and
labels=%q order by status ASC,
resolutiondate DESC`, n.conf.Project, key.Hash()))

Thanks

@jkroepke
Copy link
Contributor Author

Sure. After merge. Everyone is invited to improve.

I would not like to add more feature here. Even I understand you request.

The current PR covery already a lot of cases to test. Adding more new features results into more testing. I would prefer to add new features in separate PRs.

@holger-waschke
Copy link

Is this implementation only for Jira Cloud or can I still use the on-premise API with a PAT Authentication?

@jkroepke
Copy link
Contributor Author

jkroepke commented Dec 5, 2023

Hi @holger-waschke

I'm not able to test on-premise. But you can enter an Server API Path to an on premise instance and the authentication is still auth basic which should work on on-premise, too.

The integration does not use any cloud specific features. If https://github.com/prometheus-community/jiralert works on-premise, this integration should work, too.

I remember that Issue handling, the Jira Software Server API v2 and Cloud API v2 is the same.

But please keep in mind that this integration will always have best-effort since most users are not able to setup a on premise instance.

Copy link
Member

@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 for starting this! I haven't looked yet in details into the notifier but I suppose that it's almost a 1:1 copy from the original project?

Not a blocker but I think that it's the first notifier which needs to query the external system before actually sending the notification. It should be highlighted in the documentation IMHO.

docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/notifiers.go Outdated Show resolved Hide resolved
config/notifiers.go Outdated Show resolved Hide resolved
config/notifiers.go Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
notify/jira/jira.go Outdated Show resolved Hide resolved
jkroepke and others added 8 commits December 11, 2023 16:03
Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
@jkroepke
Copy link
Contributor Author

jkroepke commented Dec 11, 2023

Thanks for starting this! I haven't looked yet in details into the notifier but I suppose that it's almost a 1:1 copy from the original project?

No. while jiralert uses a unofficial outdated REST SDK for JIRA, I used the standard http client to setup the few http calls.

# Conflicts:
#	asset/assets_vfsdata.go
Signed-off-by: Jan-Otto Kröpke <[email protected]>
@jkroepke
Copy link
Contributor Author

The code looks good to me. Unfortunately, I cannot test in my setup, because I have a problem with the field Labels. I'll way the code be merged to include https://github.com/prometheus-community/jiralert/

I‘m open to extend the feature set after this PR got merged.

Background: it seems like AM has some review capacity issues and increase the scope involves an additional review each time. The current scope is well tested (including manual test against prod systems).

Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>

# Conflicts:
#	config/config.go
@SuperQ
Copy link
Member

SuperQ commented Jul 2, 2024

Yes, I would like to see this merged so we can iterate on additional featres.

@SuperQ SuperQ requested a review from gotjosh July 2, 2024 05:14
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Show resolved Hide resolved
@@ -37,6 +37,7 @@ require (
github.com/shurcooL/httpfs v0.0.0-20190707220628-8d4bc4ba7749
github.com/shurcooL/vfsgen v0.0.0-20200824052919-0d455de96546
github.com/stretchr/testify v1.9.0
github.com/trivago/tgo v1.0.7
Copy link
Member

Choose a reason for hiding this comment

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

json-iterator is already an indirect dependency via prometheus/client_golang so I'd say that it's ok to use it.

Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
docs/configuration.md Outdated Show resolved Hide resolved
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Some minor doc consistency nits.

docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
jkroepke and others added 5 commits July 2, 2024 21:02
Co-authored-by: Ben Kochie <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
@jkroepke
Copy link
Contributor Author

jkroepke commented Jul 2, 2024

Thanks for all the reviews.

@jkroepke
Copy link
Contributor Author

Hi all, after 2 approvals. how we should proceed?

@SuperQ
Copy link
Member

SuperQ commented Jul 12, 2024

Ping @gotjosh @simonpasquier

Copy link
Member

@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.

I'll review by the end of the week, sorry for the delay.

Copy link
Member

@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.

I'd have minor comments on making the code a bit more compact but nothing that should prevent merging this PR. Thanks again for your patience!
I'll also look into building a new RC for Alertmanager.

@simonpasquier simonpasquier merged commit a509c14 into prometheus:main Jul 31, 2024
11 checks passed
@jkroepke jkroepke deleted the jira branch July 31, 2024 15:27
TheMeier pushed a commit to TheMeier/alertmanager that referenced this pull request Sep 29, 2024
* Initial jira integration

Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Co-authored-by: Ben Kochie <[email protected]>
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.

9 participants