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

feat: Default repo-token to GitHub token #642

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

shrink
Copy link

@shrink shrink commented Feb 5, 2023

The absence of a default value for repo-token means that this action may start to fail long after it has been implemented by a developer: the developer adds the action to their workflow (without a token because it's optional) and then, at some point in the future, their workflows may randomly start to fail because they've hit the unauthenticated rate-limit.

The absence of a default value for  `repo-token` means that this action may start to fail long after it has been implemented by a developer: the developer adds the action to their workflow (without a token because it's optional) and then, at some point in the future, their workflows may *randomly* start to fail because they've hit the unauthenticated rate-limit.
@per1234 per1234 self-assigned this Feb 7, 2023
@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Feb 7, 2023
Copy link

@LeoDog896 LeoDog896 left a comment

Choose a reason for hiding this comment

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

LGTM! This seems to be the standard way to do it.

@shrink
Copy link
Author

shrink commented Feb 11, 2023

@LeoDog896 Yep, sorry, I should have noted that this is the standard approach.

Just to catch one concern that might come up: although adding github.token as the default value does that mean by default this action will now be given the active token, it's actually already possible for any action to access the token. The use of github.token as the default value for repo-token is the idiomatic way to pass the token because it's leveraging the standard GitHub Action configuration flow, but other approaches are available, like bypassing Action configuration altogether and instead just pulling it from the environment -- i.e: process.env.GITHUB_TOKEN.

Important: An action can access the GITHUB_TOKEN through the github.token context even if the workflow does not explicitly pass the GITHUB_TOKEN to the action. As a good security practice, you should always make sure that actions only have the minimum access they require by limiting the permissions granted to the GITHUB_TOKEN. For more information, see "Permissions for the GITHUB_TOKEN." ...via GitHub Actions / Security guides / Automatic token authentication.

Separate from the above, while reviewing this Pull Request again, I've noticed I missed making a change to the README. At the moment the README says:

repo-token
(Optional) GitHub access token used for GitHub API requests. Heavy usage of the action can result in workflow run failures caused by rate limiting. GitHub provides a more generous allowance for Authenticated API requests.
It will be convenient to use ${{ secrets.GITHUB_TOKEN }}.

I think something like this might be clearer in light of this change:

repo-token
(Optional) GitHub access token used for GitHub API requests. When no repo-token is provided, the Workflow token will be used by default.

Let me know if that sounds like a more helpful description, and I'll update this Pull Request to include the README change too :)

Thanks!

Copy link
Contributor

@per1234 per1234 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 your PR @shrink!

I think something like this might be clearer in light of this change:

I think the best thing is to follow Arduino's established format for documenting default input values, as is already used for the version input.

Here is an example of how the default token value was documented in one of Arduino's other actions:

https://github.com/arduino/report-size-deltas#github-token

@CLAassistant
Copy link

CLAassistant commented Feb 11, 2023

CLA assistant check
All committers have signed the CLA.

@shrink
Copy link
Author

shrink commented Feb 11, 2023

I think the best thing is to follow Arduino's established format for documenting default input values, as is already used for the version input.

Thank you very much, I agree, consistency is best! I've added the established format.

README.md Outdated Show resolved Hide resolved
Co-authored-by: per1234 <accounts@perglass.com>
@per1234 per1234 dismissed their stale review February 14, 2023 13:08

Requested changes have been made. Thanks!

@shrink shrink closed this by deleting the head repository Jul 4, 2023
@andreynering
Copy link
Contributor

Closed by mistake? This still seems to be an useful change to me. @shrink @per1234

@shrink shrink reopened this Jul 5, 2023
@shrink
Copy link
Author

shrink commented Jul 5, 2023

@andreynering you're right, thank you for catching the mistake. I deleted some unused forks (or so I thought): I missed that this Pull Request was still open. I've asked GitHub to restore the repository so the Pull Request is now re-opened.

Copy link

@matfax matfax left a comment

Choose a reason for hiding this comment

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

It could be improved to account for enterprise servers, but this can be done in a future update if need be, since other code sections might be affected by this as well.

@@ -9,6 +9,7 @@ inputs:
repo-token:
description: "Token with permissions to do repo things"
required: false
default: "${{ github.token }}"
Copy link

Choose a reason for hiding this comment

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

Could be replaced with default: ${{ github.server_url == 'https://github.com' && github.token || '' }} as in actions/setup-go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants