-
Notifications
You must be signed in to change notification settings - Fork 59
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
Define new --version and --autoclone flags #300
Conversation
Looks good to me. Left a few minor comments, mostly around error messages. |
1f05c7d
to
88a7202
Compare
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.
Looks good to me.
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.
This is looking good. I'm still slightly unsure about the utility of 'editable'. Otherwise and with a couple of small copy edits, this looks great.
Change default from 'editable' to 'latest'?
For the backend create
scenario, the default behaviour we're creating requires that the first and second invocations use different flags:
# This creates the new draft version
fastly backend create --version=active --autoclone
# This uses the draft version created in the previous step
fastly backend create
If you thought you maybe already had a draft but didn't and tried to run the first command without flags, it would error, so fair enough. But if you thought you didn't have a draft and actually you do, then that first command will create another draft from the active, possibly losing changes you've made to your existing draft.
If we were to change the default behaviour so that if you don't specify a version, you get latest
instead of active
, then this improves the experience in all scenarios, because regardless of whether you have an open draft or not, your command is the same:
# This clones from latest *only if latest is not editable*
fastly backend create --autoclone
# This uses an editable draft, either because it already existed or was created by the previous command
fastly backend create --autoclone
Drop 'editable'?
I'm decidedly unconvinced about the utility of 'editable' at all, actually. If the latest version is a draft, then that's likely the one you want to edit, and latest
covers that. If the latest version is not a draft, then that leaves these possiblities:
- it might the active, in which case auto-selecting an editable version older than the active is dangerous and likely a mistake (so we prevent you from doing it).
- It is locked and there's no draft version newer than the active, in which case we still can't auto select an editable version.
- it is locked, but there is a draft version between the latest and the active. This will only happen if you have previously activated the latest, and then rolled back to latest minus 2, leaving latest minus 1 as editable because it has never been activated. This seems like a vanishingly rare scenario and I feel like just requiring the version to be given explicitly in this scenario is perfectly fine.
Are there other scenarios in which editable
is useful that I'm missing here?
Special case for 'compute deploy'?
Are we retaining the special case for deploy that autoclones and activates by default? Is there an opportunity to hook it into the new code rather than retaining dedicated code for that behaviour? Or maybe we wait and do this if/when we ship --activate
?
Problem: Identifying the correct service version to use is a manual process which slows down a user's workflow.
Solution: Overload the
--version
flag and implement a new--autoclone
flag (fixes: #141).Notes: This PR defines the new flags (and tests for the internal behaviours) but doesn't integrate them yet (see follow-up PRs).
The implementation can be summarised as follows:
--version
will call aserviceVersion.Parse
function.--version
flag is set with the valuelatest
:--autoclone
might need to be specified too.--version
flag is set with the valueactive
:--version
flag is set with a numerical value (e.g.1
,2
,3
etc):--version
flag is set with the valueeditable
:--version
flag is not set (or an empty value is provided):--autoclone
is defined, we call aautoclone.Parse
function.autoclone.Parse
function should only be called inside of commands where an 'editable' version is required.--autoclone
flag should only be defined on commands where an 'editable' version is required.Example Scenarios
Version not set, no editable versions available
The user has a service with a single version that is 'active'. They execute
backend create
without passing the--version
flag.In this scenario the CLI would internally call
getLatestEditableVersion
and, because there are no editable versions available, it would return an error message such as "error retrieving an editable service version".The resolution for the user would be to pass both the
--version
flag and the--autoclone
flag. The value of the--version
flag could be eitherlatest
,active
or1
. With--autoclone
also specified this would cause a new editable service version2
to be created, and the backend created within that new version.On subsequent calls to
backend create
the user could then omit the--version
and--autoclone
flags as the CLI would infer callinggetLatestEditableVersion
and (unless the user had 'activated' the newly cloned version) would result in the newly cloned/editable version being identified and the new backend resource created within it.Alternatively, on subsequent calls the user could specify either
--version=latest
or--version=editable
. Because each CLI invocation will acquire an updated versions list, which is then sorted in descending order, this should ensure (regardless of whether the valuelatest
oreditable
was provided) the version returned was the newly cloned/editable version and thus--autoclone
wouldn't be needed on the subsequent calls.Version not set, editable version available
The user has a service with a single version that is editable (i.e. it is neither 'locked' nor 'active'). They execute
backend create
multiple times without passing the--version
flag.In this scenario the CLI would internally call
getLatestEditableVersion
on each CLI invocation (resulting in the editable version1
being identified each time), which means the three backends would be created within the same editable version.Alternatively, the user could prefer to be explicit about their intention and for each CLI invocation they could pass either
--version=editable
, which also would internally callgetLatestEditableVersion
, or--version=latest
. Both would result in the editable version1
being identified each time.