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

Provide a TypeScript version of the Action #124

Merged
merged 47 commits into from
Apr 23, 2024
Merged

Provide a TypeScript version of the Action #124

merged 47 commits into from
Apr 23, 2024

Conversation

lucperkins
Copy link
Member

@lucperkins lucperkins commented Apr 21, 2024

This PR switches the Action from a Bash script to TypeScript-based logic using the detsys-ts library.

@lucperkins lucperkins marked this pull request as ready for review April 21, 2024 21:56
@lucperkins lucperkins requested a review from grahamc April 21, 2024 21:56
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

Looking pretty good, a few nits, and then let's do a test on a few repos.

ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
@grahamc
Copy link
Member

grahamc commented Apr 22, 2024

Let's update the action's options from flakehub-push-* to source-*. Make sure to keep the old ones around, but mark them undocumented.

@lucperkins lucperkins requested a review from grahamc April 22, 2024 15:37
@lucperkins
Copy link
Member Author

@grahamc How would you suggest doing that, given that fetchExecutable() in detsys-ts handles all of that magically?

@grahamc
Copy link
Member

grahamc commented Apr 23, 2024

@grahamc How would you suggest doing that, given that fetchExecutable() in detsys-ts handles all of that magically?

fetchExecutable will look for source-* parameters automatically, and then fall back to the legacy prefix only if the source-* parameters aren't set. If the user uses a legacy option, they're issued a warning to switch:

ts/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@colemickens colemickens left a comment

Choose a reason for hiding this comment

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

seems like CI is unhappy, but looks fine to me.

ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Show resolved Hide resolved
FLAKEHUB_PUSH_SPDX_EXPRESSION: ${{ inputs.spdx-expression }}
FLAKEHUB_PUSH_ERROR_ON_CONFLICT: ${{ inputs.error-on-conflict }}
FLAKEHUB_PUSH_INCLUDE_OUTPUT_PATHS: ${{ inputs.include-output-paths }}
# Also GITHUB_REPOSITORY, GITHUB_REF_NAME, GITHUB_TOKEN, ACTIONS_ID_TOKEN_REQUEST_TOKEN, ACTIONS_ID_TOKEN_REQUEST_URL
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to keep around the documentation of these "implicit" env vars we depend on in the new TS version.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure we really need that, to be honest. These vars only set by the JS and are thus completely opaque to users.

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

After my comments above are addressed, LGTM.

@lucperkins lucperkins merged commit 7f4cf85 into main Apr 23, 2024
6 checks passed
@lucperkins lucperkins deleted the detsys-ts branch April 23, 2024 15:41
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