Add a GitHub Action for installing slsa-verifier. #246

kpk47 · 2022-09-02T01:49:41Z

This is a rewrite of #233 in Typescript.

laurentsimon

looks good. Left a few comment. I think this is the "allow installation of any version". Heard back from Ian whether he's OK with using the @v1.2.3 opption instead?

.github/workflows/test_install.yml

actions/installer/dst/index.js

actions/installer/src/index.ts

kpk47 · 2022-09-02T22:31:32Z

@laurentsimon I spoke with Ian about pinning by release tags, and he's fine with it. Unfortunately, I've written a bunch of versions of this action and decided that I like the version with a version input and an explicit LATEST_VERSION in the javascript. Here's my reasoning. LMK if you disagree.

Pros

Users can still pin to a release tag if they want. The version input is optional, and pinning to a release tag will install whichever version was latest at the time of release (i.e. @1.2.3 will install v1.2.3).
If we have an explicit LATEST_VERSION in the code, users can pin to a commit hash. GH's documentation on using public actions recommends pinning to a commit SHA rather than a release version or main, so I think it's important we support that use case.
It's easier to write the action if it takes the version as input. action_path and action_ref are not part of the subset of the GH context that's available to Javascript actions, so getting the version that way requires an ugly hack with a second (composite) action. I wrote that version and discarded it because my intuition is that it would be brittle.
It's easier to test the action if it takes the version as an input since we can use a matrix to cover all supported release versions. We cannot parse the version from a local action (it's always main at whatever commit you have checked out), so we'd need to use a second repo for running tests. I don't think it's worth the hassle.
After looking through the GHA Marketplace, I realized that it would be very unusual if we only allowed choosing the installed version by pinning the action to a release tag. Every other *-setup action I found either supported taking the version as an input or only supported installing the latest release. I think that being unusual in this way would be helpful for adoption.

Cons

We have to keep the LATEST_VERSION constant up to date, which most likely means adding an extra step to our release process.

laurentsimon · 2022-09-02T22:39:03Z

@laurentsimon I spoke with Ian about pinning by release tags, and he's fine with it. Unfortunately, I've written a bunch of versions of this action and decided that I like the version with a version input and an explicit LATEST_VERSION in the javascript. Here's my reasoning. LMK if you disagree.

Pros

Users can still pin to a release tag if they want. The version input is optional, and pinning to a release tag will install whichever version was latest at the time of release (i.e. @1.2.3 will install v1.2.3).

If we have an explicit LATEST_VERSION in the code, users can pin to a commit hash. GH's documentation on using public actions recommends pinning to a commit SHA rather than a release version or main, so I think it's important we support that use case.

We can support this pinning by hash by getting the hash from the GITHUB_ACTION_PATH env variable, iterating over releases and finding the one that corresponds to the hash. Note that it would be fine to have a first version that only supports tag pins. Not a problem.

It's easier to write the action if it takes the version as input. action_path and action_ref are not part of the subset of the GH context that's available to Javascript actions, so getting the version that way requires an ugly hack with a second (composite) action. I wrote that version and discarded it because my intuition is that it would be brittle.

action_path is accessible thru GITHUB_ACTION_PATH env variable, and it contains the ref, which is either a tag or as hash.

It's easier to test the action if it takes the version as an input since we can use a matrix to cover all supported release versions. We cannot parse the version from a local action (it's always main at whatever commit you have checked out), so we'd need to use a second repo for running tests. I don't think it's worth the hassle.

We can use an API to list the version, and dynamically generate the matrix, as in https://michaelheap.com/dynamic-matrix-generation-github-actions/.

After looking through the GHA Marketplace, I realized that it would be very unusual if we only allowed choosing the installed version by pinning the action to a release tag. Every other *-setup action I found either supported taking the version as an input or only supported installing the latest release. I think that being unusual in this way would be helpful for adoption.

That's the main argument against.

Cons

We have to keep the LATEST_VERSION constant up to date, which most likely means adding an extra step to our release process.

Let's enjoy the week-end. I know re-writing so many times is frustrating. Let's sync next week.
Thanks again for the hard work you've put into this. Maybe we've been a bit unlucky with the choice of the first PR :/

laurentsimon

Thanks!
Just added a nit to rename files for pre-submit, and we're good to merge

.github/workflows/test_install.yml

ianlewis

Looks good!

.github/workflows/pre-submit.yml

actions/installer/README.md

actions/installer/spec/index.test.ts

actions/installer/src/index.ts

actions/installer/README.md

laurentsimon

I thought we'd use the GITHUB_ACTION_PATH to determine the version to download?

kpk47 · 2022-09-08T20:16:41Z

@laurentsimon GITHUB_ACTION_PATH is only available to composite actions (https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables).

laurentsimon · 2022-09-08T20:44:41Z

@laurentsimon GITHUB_ACTION_PATH is only available to composite actions (https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables).

ouch, all these strange corner cases. Good find!
One hack is to declare a composite action and have a single step that calls node code.js. Does this work?

Note: GH are working on better APIs for Actions / re-usable workflows to determine their identity / ref, so later we should be able to use it.

actions/installer/action.yml

kpk47 · 2022-09-15T15:39:28Z

Okay, I think I've address all of your comments. PTAL and then I think we're ready to merge.

laurentsimon · 2022-09-15T15:43:32Z

Please re-base

laurentsimon · 2022-09-15T15:45:16Z

actions/installer/README.md

+## Usage
+
+To install the latest version, use:
+


Let's remove this. We don't want to encourage this

laurentsimon · 2022-09-15T15:45:57Z

actions/installer/src/index.ts

+  }
+
+  // If actionRef is a commit SHA, then find the associated version number.
+  const shaRe = /^[a-f\d]{40}$/;


This is really nice!

Thanks! I copied it from you (slsa-framework/slsa-github-generator#86). 😅

laurentsimon · 2022-09-15T15:46:57Z

actions/installer/action.yml

+      shell: bash
+      run: npm ci
+
+    - name: Run build and tests


forgot to remove the test in the main action?

.github/workflows/pre-submit.actions.yml

actions/installer/README.md

ianlewis · 2022-09-15T23:23:20Z

LGTM, pending doc changes that @laurentsimon commented on, and merging the conflicting pre-submit.yml changes.

kpk47 · 2022-09-16T07:09:33Z

I think I've fixed the merge conflicts, but I can't get GitHub to recognize it. I dropped the changes in pre-submit.yml in favor of a straight rename, so I'm not sure what else I can do.

laurentsimon · 2022-09-16T15:25:31Z

yeah things get weird when re-basing and files that are deleted have some updates. Have you tried git rm the pre-submit.yml to see if it helps?

kpk47 · 2022-09-19T19:20:09Z

Okay, I think I got it. For posterity, the fix was re-adding pre-submit.yml, merging in changes from main, removing pre-submit.cli.yml, and then re-doing the name change from pre-submit.yml to pre-submit.cli.yml.

laurentsimon · 2022-09-19T22:11:02Z

actions/installer/README.md

+
+See https://github.com/slsa-framework/slsa-verifier/releases for the list of available `slsa-verifier` releases.
+
+For a full example workflow, see [../../.github/workflows/test_installer.yml](https://github.com/slsa-framework/slsa-verifier/.github/workflows/test_installer.yml).


I think think this file exists anymore, does it? Let's remove this line. You can send a PR to add an example to this README later, including a link to it from the main README.

kpk47 marked this pull request as ready for review September 2, 2022 01:50

laurentsimon reviewed Sep 2, 2022

View reviewed changes

.github/workflows/test_install.yml Outdated Show resolved Hide resolved

actions/installer/dst/index.js Outdated Show resolved Hide resolved

actions/installer/src/index.ts Outdated Show resolved Hide resolved

kpk47 force-pushed the ts branch from f328e68 to 82db5e3 Compare September 3, 2022 00:03

laurentsimon approved these changes Sep 8, 2022

View reviewed changes

.github/workflows/test_install.yml Outdated Show resolved Hide resolved

ianlewis approved these changes Sep 8, 2022

View reviewed changes

laurentsimon reviewed Sep 8, 2022

View reviewed changes

actions/installer/README.md Outdated Show resolved Hide resolved

laurentsimon reviewed Sep 8, 2022

View reviewed changes

laurentsimon reviewed Sep 15, 2022

View reviewed changes

actions/installer/action.yml Outdated Show resolved Hide resolved

laurentsimon reviewed Sep 15, 2022

View reviewed changes

kpk47 force-pushed the ts branch from a56d945 to f153571 Compare September 15, 2022 17:16

laurentsimon reviewed Sep 15, 2022

View reviewed changes

actions/installer/README.md Outdated Show resolved Hide resolved

laurentsimon reviewed Sep 15, 2022

View reviewed changes

actions/installer/README.md Outdated Show resolved Hide resolved

ianlewis approved these changes Sep 15, 2022

View reviewed changes

kpk47 force-pushed the ts branch 3 times, most recently from 6ddb1f1 to e945c99 Compare September 16, 2022 07:06

kpk47 force-pushed the ts branch 5 times, most recently from e9776c3 to 03fd470 Compare September 19, 2022 19:17

laurentsimon reviewed Sep 19, 2022

View reviewed changes

laurentsimon enabled auto-merge (squash) September 19, 2022 22:12

auto-merge was automatically disabled September 19, 2022 22:19
Head branch was pushed to by a user without write access

kpk47 force-pushed the ts branch 3 times, most recently from 632340c to a0f47b9 Compare September 19, 2022 22:30

laurentsimon enabled auto-merge (squash) September 19, 2022 22:37

auto-merge was automatically disabled September 22, 2022 16:58
Head branch was pushed to by a user without write access

Add a GitHub Action for installing slsa-verifier.

e8c2d2a

kpk47 force-pushed the ts branch from bca6ae3 to e8c2d2a Compare September 22, 2022 17:24

laurentsimon enabled auto-merge (squash) September 22, 2022 17:36

laurentsimon merged commit b9c3c9d into slsa-framework:main Sep 22, 2022

kpk47 deleted the ts branch September 22, 2022 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a GitHub Action for installing slsa-verifier. #246

Add a GitHub Action for installing slsa-verifier. #246

kpk47 commented Sep 2, 2022

laurentsimon left a comment

kpk47 commented Sep 2, 2022

laurentsimon commented Sep 2, 2022 •

edited

Loading

laurentsimon left a comment

ianlewis left a comment

laurentsimon left a comment

kpk47 commented Sep 8, 2022

laurentsimon commented Sep 8, 2022

kpk47 commented Sep 15, 2022

laurentsimon commented Sep 15, 2022

laurentsimon Sep 15, 2022

kpk47 Sep 15, 2022

laurentsimon Sep 15, 2022

kpk47 Sep 15, 2022

laurentsimon Sep 15, 2022

kpk47 Sep 15, 2022

ianlewis commented Sep 15, 2022

kpk47 commented Sep 16, 2022

laurentsimon commented Sep 16, 2022

kpk47 commented Sep 19, 2022

laurentsimon Sep 19, 2022 •

edited

Loading


		See https://github.com/slsa-framework/slsa-verifier/releases for the list of available `slsa-verifier` releases.

		For a full example workflow, see [../../.github/workflows/test_installer.yml](https://github.com/slsa-framework/slsa-verifier/.github/workflows/test_installer.yml).

Add a GitHub Action for installing slsa-verifier. #246

Add a GitHub Action for installing slsa-verifier. #246

Conversation

kpk47 commented Sep 2, 2022

laurentsimon left a comment

Choose a reason for hiding this comment

kpk47 commented Sep 2, 2022

laurentsimon commented Sep 2, 2022 • edited Loading

laurentsimon left a comment

Choose a reason for hiding this comment

ianlewis left a comment

Choose a reason for hiding this comment

laurentsimon left a comment

Choose a reason for hiding this comment

kpk47 commented Sep 8, 2022

laurentsimon commented Sep 8, 2022

kpk47 commented Sep 15, 2022

laurentsimon commented Sep 15, 2022

laurentsimon Sep 15, 2022

Choose a reason for hiding this comment

kpk47 Sep 15, 2022

Choose a reason for hiding this comment

laurentsimon Sep 15, 2022

Choose a reason for hiding this comment

kpk47 Sep 15, 2022

Choose a reason for hiding this comment

laurentsimon Sep 15, 2022

Choose a reason for hiding this comment

kpk47 Sep 15, 2022

Choose a reason for hiding this comment

ianlewis commented Sep 15, 2022

kpk47 commented Sep 16, 2022

laurentsimon commented Sep 16, 2022

kpk47 commented Sep 19, 2022

laurentsimon Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

laurentsimon commented Sep 2, 2022 •

edited

Loading

laurentsimon Sep 19, 2022 •

edited

Loading