-
Notifications
You must be signed in to change notification settings - Fork 39
Rename version input to cli-version
#115
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
| required: true | ||
| type: boolean | ||
| version: | ||
| cli-version: |
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.
hmm if we are already recgonizing that the SDK is going to be supported here soon enough, is this change even necessary? Seems like more work as when the SDKs get introduced this has to be fixed again
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.
Great question!
I thought introducing version would be a nice to have feature, especially as we give it for free as of using op-cli-installer package.
Having version input in the action would be useful in case latest CLI release has a regression, so users can start using older version of the CLI without the issue.
On another hand, as you fairly mentioned, as we move toward using SDK instead of CLI, that seems as additional work. Considering the fact that current load-secrets-action@v2 uses latest cli version as default, maybe we do not need to introduce this additional input at all and should remove it?
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.
Though, considering the fact that we do not know exact timeline when we switch to SDK, I feel that cli-version input this is something nice to have until that.
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.
my point is why can't users just treat version as the cli_version and we can put a comment above it instead and treat it as such until the SDK comes out. Changing it everywhere seems like a lot of work when we know we are going to migrate to the SDKs soon and then change this again.
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.
I see your point.
Especially as we already have a description in the action.yml
version:
description: Specify which 1Password CLI version to install. Defaults to "latest".
default: "latest"
Ok, let's close this PR and stick with version
|
After discussion decided to not merge it to not bring more changes as we already have a description for the |
This PR renames
versioninput tocli-versionto make it more obvious about which version we are talking about.It defaults to
latestif not provided.In the future releases when we will migrate from CLI to SDK, the
cli-versionwill be deprecated. And we will introduce a warning about future depreciation, close to that moment.