-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/pagerduty: Validate credentials #12854
Conversation
client := pagerduty.NewClient(c.Token) | ||
|
||
// Validate the credentials by calling the abilities endpoint, | ||
// if we get a 401 response back we return an error to the user | ||
if _, err := client.ListAbilities(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will all account types be able to access the Abilities endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All plans (except for the Lite
plan which doesn't allow API access) should be able to call the Abilities endpoint.
I've tested this on a Basic
and a Standard
plan and I assume the Enterprise
plan should work as well considering it has everything a Standard
plan has.
I guess one thing we could do is adding something like SkipValidate
which would allow the user to skip the validation? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a ignore_credentials_validation
field (similar to the one in the AWS provider) so the user can skip the validation against the PagerDuty API :)
|
||
"skip_credentials_validation": { | ||
Type: schema.TypeBool, | ||
Optional: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we need to define a default here? We are using d.Get - what if the value isn't found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch :) I'll push a fix for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skip_credentials_validation
should now default to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! thanks :)
LGTM!
|
* Validate credentials * Add ability to skip validation * Update provider documentation * invalidCredentials -> invalidCreds * Include original error message * Update description for skip_credentials_validation * Add config test * set skip_credentials_validation default to false
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This PR adds token validation to the PagerDuty provider.
Previously if the token was missing (empty string) or invalid,
an error message like this was displayed to the user:
To make the error a bit more user-friendly we instead display this error message if the token is missing or invalid:
No token
Invalid token