Skip to content

Ignore scope suggestions for http 422#4809

Merged
mislav merged 3 commits intocli:trunkfrom
despreston:4624-scopes
Dec 1, 2021
Merged

Ignore scope suggestions for http 422#4809
mislav merged 3 commits intocli:trunkfrom
despreston:4624-scopes

Conversation

@despreston
Copy link
Contributor

HTTP 422 messages are for validation errors, but OAUTH permissions
suggestions get printed anyways. Most times, the user probably has the
right permissions. This fix adds the check to avoid printing a confusing
message.

Fixes #4624

HTTP 422 messages are for validation errors, but OAUTH permissions
suggestions get printed anyways. Most times, the user probably has the
right permissions. This fix adds the check to avoid printing a confusing
message.

Fixes cli#4624
@despreston despreston marked this pull request as ready for review November 24, 2021 20:13
@despreston despreston requested a review from a team as a code owner November 24, 2021 20:13
@despreston despreston requested review from vilmibm and removed request for a team November 24, 2021 20:13
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Good catch!

cmd/gh/main.go Outdated
} else if strings.Contains(err.Error(), "Resource protected by organization SAML enforcement") {
fmt.Fprintln(stderr, "Try re-authenticating with: gh auth refresh")
} else if msg := httpErr.ScopesSuggestion(); msg != "" {
} else if msg := httpErr.ScopesSuggestion(); msg != "" && httpErr.StatusCode != 422 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move the "not 422" check into ScopesSuggestion function itself? It's where it performs the check that this should be done for HTTP 4xx in the first place. Thank you!

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Thanks! I have pushed an additional commit to move the check to where I meant; sorry that I didn't explain myself too well.

@mislav mislav enabled auto-merge (squash) December 1, 2021 18:07
@mislav mislav merged commit 8f9548f into cli:trunk Dec 1, 2021
@despreston despreston deleted the 4624-scopes branch December 1, 2021 21:05
@VictorBatta VictorBatta mentioned this pull request Dec 4, 2021
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.

Erroneous error message when using invalid license shorthand

2 participants