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

Allow to Use Custom Git Actor for Bundled JS Output #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DavideIadeluca
Copy link

@DavideIadeluca DavideIadeluca commented Oct 18, 2024

Counterpart to flarum/framework#4078 and flarum/framework#4099

Fixes #0000
Please see the description in flarum/framework#4078 for why this PR was opened.

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

QA

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@DavideIadeluca DavideIadeluca marked this pull request as ready for review October 19, 2024 17:06
Copy link
Member

@luceos luceos left a comment

Choose a reason for hiding this comment

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

This seems rather innocent. I still prefer seeing a review by @SychO9 before merging.

Comment on lines -64 to +69
const token = core.getInput('github_token', { required: true, trimWhitespace: true });

debugLog(`** Pushing commit`);

await git.addRemote('upstream', `https://${process.env.GITHUB_ACTOR}:${token}@github.com/${process.env.GITHUB_REPOSITORY}.git`);
await git.addRemote('upstream', `https://github-actions:${process.env.GITHUB_TOKEN}@github.com/${process.env.GITHUB_REPOSITORY}.git`);
Copy link
Member

Choose a reason for hiding this comment

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

is this changed because the github token parameter is no longer necessary?

Copy link
Author

Choose a reason for hiding this comment

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

The token is still necessary, but it is fetched in another way. If a workflow provides a custom git_actor_token, then that token will be used instead of the default GITHUB_TOKEN.

REUSABLE_frontend.yml:

steps:
    - name: Check out code
      uses: actions/checkout@v4
      with:
        token: ${{ secrets.git_actor_token != '' && secrets.git_actor_token || secrets.GITHUB_TOKEN }}

Copy link
Member

Choose a reason for hiding this comment

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

sorry I don't get it. there is no git_actor_token for this action build though, it seems like the github_token parameter is no longer used after this PR, but is still documented. Though I still can't tell if it's no longer necessary here because the new upstream link doesn't require passing a token from the origin repo:

https://${process.env.GITHUB_ACTOR}:${token}@github.com/${process.env.GITHUB_REPOSITORY}.git

vs

https://github-actions:${process.env.GITHUB_TOKEN}@github.com/${process.env.GITHUB_REPOSITORY}.git

Copy link
Author

Choose a reason for hiding this comment

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

The github_token will continue to be used after this PR.

Let's say an extension provides the new git_actor_token secret to the REUSABLE_frontend.yml workflow of core. At https://github.com/flarum/framework/pull/4099/files#diff-8f0cab030414f9e64c7444c9011370f3435c52893686c91babdb72810568b0dcR136-R137 we are checking if that token is provided or not. If yes, the GITHUB_TOKEN will be reassigned with the value of the token provided by the extension. If not, the default GITHUB_TOKEN will be used.

About the upstream link; If I recall correctly, It didn't actually make a difference if I used ${process.env.GITHUB_ACTOR} or github-actions: for the upstream link during testing. Using github-actions: felt cleaner to me.

Does that make sense?

Copy link
Member

@SychO9 SychO9 Nov 2, 2024

Choose a reason for hiding this comment

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

@DavideIadeluca sorry but you keep linking to the part of the workflow about actions/checkout, but this is about actions/build

https://github.com/flarum/framework/pull/4099/files#diff-8f0cab030414f9e64c7444c9011370f3435c52893686c91babdb72810568b0dcR165

  1. we are still passing the same github_token, even though the changes here remove that parameter
  2. github_token is still documented in this repository as a usable parameter, but the line the comments are directed at, remove its usage.

I am still confused whether github_token the parameter for this action flarum/action-build@v4 which is this repository (not actions/checkout@v4) is still needed/can be completely removed, or why the removal is done.

I understand everything else about the git actor, just the github_token parameter is still confusing to me. As this PR leaves it in a state of documented and supplied by the fremework but not actually used by the action itself

I may be forgetting something, what's the relation between the token passed to the checkout action and this build action?

 steps:
      - name: Check out code
        uses: actions/checkout@v4
        with:
          token: ${{ secrets.git_actor_token != '' && secrets.git_actor_token || secrets.GITHUB_TOKEN }}

...

- name: JS Checks & Production Build
        uses: flarum/action-build@v4
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}

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.

4 participants