Skip to content

fix(CLI releases docs): Add additional required scope for set-commits #829

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

Merged
merged 2 commits into from
Mar 12, 2019

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Mar 11, 2019

Currently, the CLI authentication docs only list two required scopes:

image

But as it turns out, you can't use set-commits unless you also have org:readscope:

This PR adds the missing scope and reorganizes the text a bit:

image

@lobsterkatie lobsterkatie requested a review from markstory March 11, 2019 19:48
Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good to me content wise. Does this need to be an alert or could it be part of the first paragraph?

@lobsterkatie
Copy link
Member Author

I put it as an alert because it's a "you gotta do me before any of the rest of this will work" kind of thing. Didn't want it to be missed by someone skimming, and other permissions stuff is called out this way:

image

@lobsterkatie
Copy link
Member Author

Another option is to just add org:read to the initial list of scopes we require (even though it's not required for all CLI commands).

@markstory
Copy link
Member

Including org:read as a default permission is a good option. It sets people up for success later and saves them having to backtrack to update permissions later on.

@lobsterkatie lobsterkatie force-pushed the kmclb-add-required-scope-for-set-commits branch from ad3cf8c to a65528d Compare March 11, 2019 22:23
@lobsterkatie
Copy link
Member Author

Okay - pulled it out of the releases page and added it into the default config. Since I was there anyway, also made the edits I was planning to stick into a different PR. Lemme know what you think.

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@lobsterkatie
Copy link
Member Author

Great, thanks. Caught one typo (in two spots) - fixed and will merge once the tests pass.

@lobsterkatie lobsterkatie changed the title fix(CLI releases docs): Add note re: additional required scope for set-commits fix(CLI releases docs): Add additional required scope for set-commits Mar 12, 2019
@lobsterkatie lobsterkatie merged commit 65bd73e into master Mar 12, 2019
@lobsterkatie lobsterkatie deleted the kmclb-add-required-scope-for-set-commits branch March 12, 2019 19:19
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants