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: add kebab-case support with.github-token #319

Merged
merged 2 commits into from
May 31, 2023
Merged

feat: add kebab-case support with.github-token #319

merged 2 commits into from
May 31, 2023

Conversation

DariuszPorowski
Copy link
Contributor

@DariuszPorowski DariuszPorowski commented May 31, 2023

Resolves #318


Behavior

Before the change?

  • no support for kebab-case style for github token using with

After the change?

  • add kebab-case style support with.github-token

Other information

  • N/A

Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change no permission to do it ;)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@ghost ghost added this to Inbox in JS May 31, 2023
Copy link
Contributor

@gr2m gr2m 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, cheers 👍🏼

@gr2m gr2m added the Type: Bug Something isn't working as documented label May 31, 2023
@ghost ghost moved this from Inbox to Bugs in JS May 31, 2023
@gr2m
Copy link
Contributor

gr2m commented May 31, 2023

Can you run npm run lint:fix to fix the linting errors? I don't seem to have write access to your fork, otherwise I'd have done it myself

@DariuszPorowski
Copy link
Contributor Author

@gr2m hmm, interesting because PR is marked Allow edits by maintainers whatever... lint done :)

@gr2m
Copy link
Contributor

gr2m commented May 31, 2023

@gr2m hmm, interesting because PR is marked Allow edits by maintainers whatever... lint done :)

probably my fault then. I used codespace on your fork, it wouldn't let me push 🤷🏼 Anyway, cheers1

@gr2m gr2m merged commit 71dd0c1 into octokit:main May 31, 2023
7 checks passed
@github-actions
Copy link

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@DariuszPorowski DariuszPorowski deleted the 318-kebabcase branch May 31, 2023 06:56
gr2m added a commit that referenced this pull request May 31, 2023
@gr2m
Copy link
Contributor

gr2m commented May 31, 2023

Sorry it looks like it introduced a regression for users of github-script. Until we figure out what, I'll have to revert this change. See #320

@DariuszPorowski
Copy link
Contributor Author

@gr2m oh! ok

kfcampbell pushed a commit that referenced this pull request May 31, 2023
@DariuszPorowski
Copy link
Contributor Author

DariuszPorowski commented May 31, 2023

@gr2m just figured out:
actions/github-script uses with.github-token by default: https://github.com/actions/github-script/blob/main/action.yml#L11

so, I assume that the intention of the creators of the actions/github-script action is to use this for the tokens: ref: https://github.com/actions/github-script/tree/main#using-a-separate-github-token
actions/github-script is already pre-authenticated with Octokit using mentioned method: https://github.com/actions/github-script/blob/main/README.md?plain=1#L13
The issue #320 contains the code that does new own Octokit instance, which was probably not in line with the action's assumptions. Regardless of that, it's still possible but token should be utilized in the actions/github-script way like:
process.env['INPUT_GITHUB-TOKEN'] or using @actions/core (already include in this action: https://github.com/actions/github-script/blob/main/README.md?plain=1#L17): const token = core.getInput('github-token')

@gr2m
Copy link
Contributor

gr2m commented May 31, 2023

how about this: instead of adding support for the github-token, we throw a helpful error message if neither token nor github_token is set, but github-token is?

@DariuszPorowski
Copy link
Contributor Author

DariuszPorowski commented May 31, 2023

@gr2m sounds like kind of workaround... but before we jump into... just saw actions/github-script does not use @octokit/auth-action at all 😸 so it's a not root cause for #320. Example from this issue uses const { Octokit } = require("@octokit/action") so it utilize @octokit/action and it uses @octokit/auth-action. But even with that it's not a problem.
Problem starts when someone use @octokit/action inside actions/github-script and want to use own token in a different way that actions/github-script expose. Why? because actions/github-script already handle the token in with.github-token so it already inside the script, and @octokit/action can access it via @octokit/auth-action (assuming my PR is already there).
Going deeper, latest release of @octokit/action does not contain @octokit/auth-action v2.1.0 yet, so it should not fail (unless someone independently installs the latest @octokit/auth-action package).

Hypothetically if the actions/github-script action will use snake_case instead kebab-case the final effect will be the same with the current (before PR) @octokit/auth-action version.

@gr2m
Copy link
Contributor

gr2m commented May 31, 2023

Ahh I see. Yeah the way @TCarrol import @octokit/action in the script is odd, as that's what the action does by default with the global github. I'll let them know

@DariuszPorowski
Copy link
Contributor Author

Use case for @octokit/action what I see it's a bit different (https://github.com/octokit/action.js/blob/main/README.md?plain=1#L50) and makes sense, but @octokit/action inside actions/github-script looks like unnecessary overlap.

@DariuszPorowski
Copy link
Contributor Author

Hey @gr2m just following up, do you think is it ok to reopen this pr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Bug Something isn't working as documented
Projects
Archived in project
JS
  
Bugs
Development

Successfully merging this pull request may close these issues.

[FEAT]: Support with.github-token (kebab-case)
2 participants