search

CircleCI-Public / trigger-circleci-pipeline-action

Trigger a CircleCI pipeline from any GitHub Actions event.
https://github.com/marketplace/actions/trigger-circleci-pipeline
47 stars 43 forks source link

Bug: Transcompiled Source Not Added With Commit Hook #45

Closed whatisdot closed 1 year ago

whatisdot commented 1 year ago

Is there an existing issue for this?

Current behavior

The pre-commit hook transcompiles the ECMA scripts into JavaScript, but doesn't add it while the commit is in progress. I can't find any documentation or conversation about this, so it's unclear if this is intentional.

Minimum reproduction code

n/a

Steps to reproduce

  1. Checkout the main branch
  2. Run git checkout -b some-other-branch
  3. Make an edit to the index.js file and run git commit -am "make a source change" to run the pre-commit hooks

Expected behavior

???

The pre-commit hook suggests that the build should be included with the commit that is in progress, but it does not add the files in the dist/ directory.

GitHub Action Version

latest

whatisdot commented 1 year ago

@KyleTryon 👋

Can you identify if this is a bug, a feature, or something that should remain as-is?

If adding the files in dist/ to the commit in progress is a desirable change, I've also put up a PR for the change ☝️

paikattilrohin commented 1 year ago

I also want to point out that the linted version of index.js isn't staged for the commit.

KyleTryon commented 1 year ago

Hey folks, The way GHA are built is a little weird with adding the build to the main branch. Open to opinions, but what I have been doing is building and pushing a build commit before each release. The main branch does not represent what is currently "live", that should be determined by the latest release.

The precommit hook is good for "testing" in ensuring that the JS does actually build but it isn't important that it is committed since we do the build before release. I think building before release is probably the safest option as well. I agree we should document it either way.

My stance would be to not push the dist/ there is a little room for something nefarious to happen there. If your PR includes it, it should be fine, we will overwrite it when we rebuild before release.

whatisdot commented 1 year ago

Thanks for the insight, @KyleTryon.

Could you describe the build and release process a bit more? It would help me understand how this fits into the workflow.

KyleTryon commented 1 year ago

No problem @whatisdot,

Currently (open to change), for this repository we use the default branch similar to staging. We open PRs to main and squash and merge the PR with a conventional commit message. The commit message informs us mostly of when we have a breaking change to worry about.

Once we have added a new feature and any related fixes, we prepare to issue a new release. Before we issue a release, we build the app in a final PR. Building the app a final time prior to tagging ensures that it is both up-to-date and secure. After the build commit is merged, we use GitHub's Releases feature to tag the release and generate the automated release notes. The conventional commits shown in the release notes can confirm that our tag choice is appropriate according to semver.

Recap:

  1. The default branch used is used to stage the next release
  2. Pull Requests (PRs) are opened to the main branch with a conventional commit message.
  3. PRs are "squash and merged" to the main branch.
  4. Before issuing a new release, a final PR is created to build the app.
  5. After the build commit is merged, GitHub's Releases feature is used to tag the release and generate automated release notes.
  6. The conventional commits shown in the release notes can confirm that the tag choice is appropriate according to SemVer.
whatisdot commented 1 year ago

I like the conventional commit pattern recommended here, and I think that is better than the auto-generated squash merge messages created by GitHub. I'll probably raise that as a suggestion to my peers later. Good stuff 👍

The process makes sense, considering the code base is inseparable from the build. One advantage of including a build with the initial feature is that each feature can be tested independently without additional build/push/commit steps. On the other hand, there's nothing stopping contributors from creating another branch, committing the build, and pushing that to test with. It might be good to include recommendations for that bit, maybe in CONTRIBUTING.md or a separate doc specific for development technical concerns.

With that in mind, I don't want to meddle with a working process. I can close this issue/PR and advise my team push builds on a separate branch for testing purposes.