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

Add GitHub workflow for automatically updating index.js #187

Closed
wants to merge 7 commits into from

Conversation

Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Feb 25, 2024

See discussion starting at #175 (comment)

This pull request changes the workflows so that:

  • Pull requests are no longer expected to update dist/index.js (respectively the now named action/index.js)
  • Push events on the main branch will build and update action/index.js if needed, and push the changes as separate commit

To achieve this, the dist/index.js is now ignored by Git and is instead copied to action/index.js by CI. This prevents users accidentally committing that file when creating pull requests.

Here is an example workflow run which detected changes and created a commit:

Limitations:

  • The push event for updating action/index.js does not trigger any other workflows, see action documentation
  • This might need additional configuration if main is a protected branch (which it probably should be?), see action documentation
    (might depend on who is the "actor" triggering the workflow; so maybe if a member who is allowed to push to protected branches merged the pull request to main, then it works without additional configuration?)

Feedback is appreciated! If this is not an approach you want to use, feel free to close this pull request.

@Marcono1234
Copy link
Contributor Author

The "Check no dist update" workflow fails because this pull request intentionally creates the initial version of action/index.js (by renaming the existing dist/index.js).

@Marcono1234 Marcono1234 marked this pull request as ready for review March 22, 2024 22:54
@Marcono1234
Copy link
Contributor Author

@bigdaz, could you please have a look and let me know what you think about this approach and the implementation?
As mentioned above, the "Check no dist update" job fails intentionally for this PR.

@bigdaz
Copy link
Member

bigdaz commented Mar 25, 2024

Thanks for this: it looks good in general.
Is there a reason that we need to change from dist/index.js to actions/index.js? This is inconsistent with other actions I maintain.
Instead, would it be possible to have npm run build generate the outputs into a different (ignored) location, and have this workflow copy them into dist?

@Marcono1234
Copy link
Contributor Author

Is there a reason that we need to change from dist/index.js to actions/index.js?

To solve the problem this pull request tries to address, it probably needs two directories, one where the regular build is writing files to (currently dist) and one which is checked into Git (currently actions). But the actions name is arbitrary.

Instead, would it be possible to have npm run build generate the outputs into a different (ignored) location, and have this workflow copy them into dist?

Yes I think that should be possible. Maybe a name starting with dist-... might be good to keep it grouped. What about dist-temp or dist-pre or similar? Or do you have any specific name in mind?

@bigdaz
Copy link
Member

bigdaz commented Apr 1, 2024

Or do you have any specific name in mind?

Not really. We could use build since that's something Gradle devs would recognize. But it will be relatively easy to change if we can come up with something better.

@Marcono1234
Copy link
Contributor Author

Sorry for not having updated this pull request yet, I will try to do so in the next days.
Or is this still relevant now that this action will be / has been migrated to https://github.com/gradle/actions (#198)?

@bigdaz
Copy link
Member

bigdaz commented Apr 12, 2024

Thanks @Marcono1234 I've adapted this PR to implement similar functionality in https://github.com/gradle/actions/blob/main/.github/workflows/ci-update-dist.yml

It's working pretty well and has simplified the development process, so thanks a lot for the inspiration!

You're right that it's probably not worth working on it here, since the when #198 is complete there will no longer be any 'dist' directory in this repository.

@Marcono1234
Copy link
Contributor Author

Ok, I will close this pull request then. I am glad the changes proposed by this pull request were useful for you nonetheless though.

@Marcono1234 Marcono1234 deleted the workflow-build-dist branch April 14, 2024 21:12
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.

None yet

2 participants