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

Create check-dist.yml #227

Merged
merged 10 commits into from
Jun 24, 2021
Merged

Create check-dist.yml #227

merged 10 commits into from
Jun 24, 2021

Conversation

brcrista
Copy link
Contributor

@brcrista brcrista commented Jun 23, 2021

This adds a workflow to check the contents of the checked-in dist/index.js against the expected version.

Here are the bones of the issue:

  1. index.js is a generated file
  2. index.js must be checked into version control
  3. We must maintain the invariant index.js == ncc_build(repo) in the main branch

Even though index.js is produced by the build, it must always be checked into version control. While we could do something more elaborate like having a workflow that runs and updates index.js in the source branch for any PR, we would still need a check like this. I also don't think it really reduces developer friction:

  1. It adds a lot of complexity to the CI system (mainly since we have to work around the limitation that events triggered by Actions Bot don't trigger runs)
  2. Having to pull your branch back after every push is about as much work as just downloading and committing the correct index.js, but it would affect every PR and not just ones where index.js has an issue.

Diff vs. hash

We originally thought of just checking the hash of the files (a checksum). However, since the files are human-readable, a diff will provide a more informative failure message than a hash. Also for this reason, I think the output of git diff is easier to read than GNU diff. With diffing, we can also ignore line endings. Computing a diff takes more time than computing a hash, but the index.js files should be small enough that this won't matter.

Testing

@brcrista brcrista marked this pull request as ready for review June 23, 2021 19:51
@brcrista brcrista requested a review from a team as a code owner June 23, 2021 19:51
brcrista and others added 2 commits June 24, 2021 10:00
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
.github/workflows/check-dist.yml Outdated Show resolved Hide resolved
@brcrista brcrista merged commit 8d06e6c into main Jun 24, 2021
@brcrista brcrista deleted the brcrista/check-dist branch June 24, 2021 15:38
@shrink
Copy link

shrink commented Jun 25, 2021

Hi @brcrista! I recently set out to tackle this exact problem in my own Actions and started out by validating that the package was up to date (actions-is-packaged) but I eventually concluded that an automated build process is the right solution... so I set aside this approach (validating the Action is packaged) and focused on automating the build process. I encountered the problems you've mentioned (like triggering Actions) but I found solutions to all of them.

I wrote a post about how + why ("Package GitHub Actions automatically with GitHub Actions") and created an example repository. I've also added this automated packaging to my own actions (e.g: actions-assert) and it has had a meaningful impact on the quality of my development experience. If there's an appetite to bring this solution into GitHub's first-party Actions, I'm happy to contribute, either by making Pull Requests myself or you're more than welcome to scavenge anything helpful from my work so far.

ps. I noticed in your other PR actions/cache#604 that you're encountering some limitations of the package validation approach. During my own testing, I found that the least fragile approach is to use hashFiles to validate that the dist is up to date.

jobs:
  is-packaged:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - id: dist
        env:
          BUILD_HASH: ${{ hashFiles('dist') }}
        run: echo "::set-output name=hash::$BUILD_HASH"
      - run: npm install
      - run: npm run all
      - name: Test Action package is up to date
        uses: pr-mpt/actions-assert@v3
        with:
          assertion: npm://@assertions/is-equal:v1
          expected: "${{ steps.dist.outputs.hash }}"
          actual: "${{ hashFiles('dist') }}"
          error-message: "Action package (`dist`) is not up to date with latest changes, please run `npm run all`"

@brcrista
Copy link
Contributor Author

@shrink that's a nice action! I can see the value for a built-in assert: type of step, although what you have now seems pretty elegant. Just brainstorming, if this were first-class, I think it would look like:

- name: Test Action package is up to date
  assert: ${{ hashFiles('dist') }} == ${{ steps.dist.outputs.hash }}
  message: "Action package (`dist`) is not up to date with latest changes, please run `npm run all`"

which would save a few lines in the YAML. I'm not sure we have the bandwidth now to fund this feature and some of the changes would be on the service side, but I'll keep an eye out now for where it would be useful.

Also, I added a paragraph to the description to explain the choice of diffing vs. hashing.

@Rhinowiwina
Copy link

merge in

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.

5 participants