Closed edif2008 closed 7 months ago
Overall note: awesome work on this TypeScript migration, @edif2008! 👏
Everything looks very solid and works well from my testing. I left a bunch of file-specific feedback in my review yesterday and some additional comments today.
Just a few remaining general thoughts:
src/utils.ts
currently has four functions in it, so I'd be in favour of splitting these out into their own files in a src/utils/
directory. They could each then get their own test suites too under src/utils/__tests__/*.test.ts
. IMO this change would make future maintenance/refactoring easier.installCLI
and loadSecrets
into their own files in src/utils/
as well?src/index.ts
with just the run
function in it. Is there a clearer name that we can use for this function? Perhaps even just something like loadSecretsAction
, since it's representative of the main functionality for this package?installCLI
, loadSecrets
and run
as well.Happy to help wherever I can with the above list, especially the unit tests.
To conclude this review - once again, great work on this PR!
Any chance this can get merged and an update to this action released? We'd like to incorporate it into some of our pipelines, but would like to have it pull in a modern build instead of the 2.10 that is in the current "live" action.
This PR migrates most of the functionality of the GitHub Action to Typescript.
Key elements used in the migration
op-js
core.setOutput
core.exportVariable
core.setSecret
core.setPath
Migration process
6c9a28c
:entrypoint.sh
index.ts
unset_prev_secrets
unsetPrevious
validateAuth
install_cli
installCLI
extract_secrets
loadSecrets
populating_secret
extractSecret
Notes
installCLI
inindex.tx
only executes the shell script that will perform the CLI installation. No real migration has been performed for this part of the action.!process.env[envConnectHost].startsWith("http://")
no-restricted-syntax
Use https instead of http in URLs
) since we perform a check for the prefixhttp://
.const installCLI = async (): Promise<void> =>
@typescript-eslint/naming-convention
CLI
being all capitalized.expect(process.env[envConnectHost]).toBe("http://localhost:8080");
no-restricted-syntax
Use https instead of http in URLs
) since we perform a check on whether the prefixhttp://
was properly appended.