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

Move computesha256 to typescript #546

Merged

Conversation

naveensrinivasan
Copy link
Collaborator

Signed-off-by: naveensrinivasan [email protected]

- Port the computesha256 from bash to typescript.

Signed-off-by: naveensrinivasan <[email protected]>
Copy link
Collaborator

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

Thanks @naveensrinivasan
Thanks for providing the link to the template you use, I'll take a look to learn a little more too

.github/actions/compute-sha256/action.yml Outdated Show resolved Hide resolved
.github/actions/compute-sha256/.prettierignore Outdated Show resolved Hide resolved
.github/actions/compute-sha256/src/main.ts Outdated Show resolved Hide resolved
.github/actions/compute-sha256/action.yml Show resolved Hide resolved
.github/actions/compute-sha256/package.json Show resolved Hide resolved
- Updated to use the native crypto library
- Included README instructions
- Some minor nits.

Signed-off-by: naveensrinivasan <[email protected]>
@naveensrinivasan naveensrinivasan force-pushed the naveen/feat/computesha256-ts branch from 4eb8c5b to b9992ad Compare July 18, 2022 20:39
Copy link
Member

@ianlewis ianlewis left a comment

Choose a reason for hiding this comment

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

Did a quick pass and this looks pretty good.

.github/actions/compute-sha256/.prettierrc.json Outdated Show resolved Hide resolved
Signed-off-by: naveensrinivasan <[email protected]>
- Included the workflow to validate the computesha256 github action

Signed-off-by: naveensrinivasan <[email protected]>
Copy link
Collaborator

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

Please bear with me trying to reproduce. I suspect we have different versions of the tool installed.
Added a few comments

.github/workflows/check-dist-sha256.yml Outdated Show resolved Hide resolved
.github/workflows/check-dist-sha256.yml Outdated Show resolved Hide resolved
.github/actions/compute-sha256/README.md Outdated Show resolved Hide resolved
.github/actions/compute-sha256/src/main.ts Outdated Show resolved Hide resolved
.github/workflows/check-dist-sha256.yml Outdated Show resolved Hide resolved
.github/workflows/check-dist-sha256.yml Outdated Show resolved Hide resolved
.github/actions/compute-sha256/README.md Outdated Show resolved Hide resolved
.github/workflows/check-dist-sha256.yml Outdated Show resolved Hide resolved
- Included permissions for the workflow
- Renamed the workflow file
- Updated the input variable to `path`
- Changed the `run` function to non-async
- Updated the README instructions
- Included the tag for pinned SHA
@naveensrinivasan naveensrinivasan force-pushed the naveen/feat/computesha256-ts branch from 1ac1ea4 to 8bb45a9 Compare July 21, 2022 21:33
Signed-off-by: naveensrinivasan <[email protected]>
@ianlewis ianlewis mentioned this pull request Jul 22, 2022
4 tasks
Copy link
Member

@ianlewis ianlewis left a comment

Choose a reason for hiding this comment

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

LGTM

@ianlewis
Copy link
Member

NOTE: We need to mark the pre-submit as required after merge.

- Moved the action within the pre-submit.actions.yml
- Removed the redundant instructions in README

Signed-off-by: naveensrinivasan <[email protected]>
@ianlewis ianlewis enabled auto-merge (squash) July 25, 2022 04:26
@ianlewis ianlewis merged commit c88f03e into slsa-framework:main Jul 25, 2022
@naveensrinivasan
Copy link
Collaborator Author

@ianlewis @laurentsimon Thank you for all the review and feedback! Excited to have this merged!

@laurentsimon
Copy link
Collaborator

Great work @naveensrinivasan. The PR looks great!

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