-
Notifications
You must be signed in to change notification settings - Fork 13
Add Windows runner support #16
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
Conversation
…on different runners
The "ts-jest" config option "isolatedModules" is deprecated and will be removed in v30.0.0. Please use "isolatedModules: true" in /Projects/install-cli-action/tsconfig.json instead, see https://www.typescriptlang.org/tsconfig/#isolatedModules
4438e2b to
14ad11e
Compare
3c454d0 to
2358b25
Compare
Most tools expect config files to be in the root by default. With that we avoid unnecesary configuration. Also IDEs `run test` buttons (like on Goland) works by default without additional configuration, which is more convenient for developers.
83b6466 to
2a02fd3
Compare
2a02fd3 to
478be7b
Compare
edif2008
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review: ✅
The overall approach is really solid. The thing that I think can be further optimized is the installation process. Currently you've implemented branches for each OS, but I think with macOS installing from .zip instead of .pkg, it can be further simplified.
I've left other small nits.
Good point. My initial implementation used |
I'd check if there's any usage of CLI versions lower than that and if not, I'd say it's safe to make that version as a minimum supported. |
|
There are users that use CLI |
edif2008
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pipeline passing shows that the action now supports installing the CLI on Windows as well. Appreciate you rewriting this as a Typescript action. It will make maintaining this much easier.
4d85610 to
3eed3f1
Compare
This MR adds support for Windows runners.
Deiced to go with TS implementation, as it:
Notes for the reviewer
.husky,.prettieringnore,jest.config.jsetc) are copied from there.conifgas in load-secrets-action, as most of the tools (like Goland test runner etc) expect config files to be in the root of the project. With that we avoid additional configuration."dist/index.js"install-cli.shin Typescript.@1password/[email protected]useseslint@v9which requires move eslint config frompackage.jsontoeslint.config.js(see docs)How to test
Pipeline should pass. It will cover an installation of different versions on all runners including Windows.
Resolves #3