Skip to content

Fix 6112#6116

Merged
klesh merged 5 commits intoapache:mainfrom
Marco-De-Stefani:fix-6112
Sep 20, 2023
Merged

Fix 6112#6116
klesh merged 5 commits intoapache:mainfrom
Marco-De-Stefani:fix-6112

Conversation

@Marco-De-Stefani
Copy link
Copy Markdown
Contributor

Summary

Fixing #6112 and removing the typo in the version of golangci-lint

Does this close any open issues?

Closes #6112

Screenshots

Before:
image
After:
image

@keon94
Copy link
Copy Markdown
Contributor

keon94 commented Sep 20, 2023

Has PagerDuty made this change to their API recently? How come this wasn't an issue before?

@Marco-De-Stefani
Copy link
Copy Markdown
Contributor Author

@keon94 I think that Pagerduty API has not been changed, by having a quick look at the GH history I believe this has been introduced with the refactor of the Pagerduty plugin.
It looks to me that the first version of the plugin implemented in the revision eef06e9 uses the correct token.
In that revision, the connection.go file contains -->
Token string mapstructure:"token" env:"PAGERDUTY_AUTH" validate:"required" encrypt:"yes"

@keon94
Copy link
Copy Markdown
Contributor

keon94 commented Sep 20, 2023

@Marco-De-Stefani very odd. I tried running the pagerduty tests under test/e2e/manual/pagerduty with and without this change and it works for me, so I don't know what's going on. Either way, there's a better fix for this which I've pointed out in the file changes.

@Marco-De-Stefani
Copy link
Copy Markdown
Contributor Author

@Marco-De-Stefani very odd. I tried running the pagerduty tests under test/e2e/manual/pagerduty with and without this change and it works for me, so I don't know what's going on. Either way, there's a better fix for this which I've pointed out in the file changes.

I noticed that as well, it works cause at line 125 of the pagerduty_test.go file it's calling "PagerDutyAccessToken" which sets the token in the correct way :)

@keon94
Copy link
Copy Markdown
Contributor

keon94 commented Sep 20, 2023

@Marco-De-Stefani very odd. I tried running the pagerduty tests under test/e2e/manual/pagerduty with and without this change and it works for me, so I don't know what's going on. Either way, there's a better fix for this which I've pointed out in the file changes.

I noticed that as well, it works cause at line 125 of the pagerduty_test.go file it's calling "PagerDutyAccessToken" which sets the token in the correct way :)

That token object doesn't have an influence here though. The test still passes without "token=". Here's what my debugger captures - connection.Token is just the raw token.
image

@Marco-De-Stefani
Copy link
Copy Markdown
Contributor Author

@Marco-De-Stefani very odd. I tried running the pagerduty tests under test/e2e/manual/pagerduty with and without this change and it works for me, so I don't know what's going on. Either way, there's a better fix for this which I've pointed out in the file changes.

I noticed that as well, it works cause at line 125 of the pagerduty_test.go file it's calling "PagerDutyAccessToken" which sets the token in the correct way :)

That token object doesn't have an influence here though. The test still passes without "token=". Here's what my debugger captures - connection.Token is just the raw token. image

Weird, I'll try to debug that as well

Copy link
Copy Markdown
Contributor

@keon94 keon94 left a comment

Choose a reason for hiding this comment

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

LGTM

@klesh klesh merged commit 10c37ea into apache:main Sep 20, 2023
@klesh
Copy link
Copy Markdown
Contributor

klesh commented Sep 20, 2023

Hi, Nice work, thanks for your contribution, would you like to cherry-pick it back to release v0.19?

@Marco-De-Stefani Marco-De-Stefani deleted the fix-6112 branch September 20, 2023 11:10
klesh pushed a commit that referenced this pull request Sep 21, 2023
* fixing issue 6112

* fix typo

* fix PR comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug][Pagerduty] Unable to collect incidents with API token

3 participants