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

Define new --version and --autoclone flags #300

Merged
merged 18 commits into from
Jun 7, 2021

Conversation

Integralist
Copy link
Collaborator

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=VERSION   Number of service version, 'latest', 'active'
                    or 'editable'
--autoclone         Automatically clone the identified service
                    version if it's 'locked' or 'active'
  • Every command that implements --version will call a serviceVersion.Parse function.
    • This function makes an API call for a list of all available service versions.
    • The returned list is sorted in descending order (i.e. newest versions first).
    • If the --version flag is set with the value latest:
      • We consider the selected version to be the latest version available.
      • The version could be 'active' or 'locked', so --autoclone might need to be specified too.
      • We do not expect the returned version to be older than the last active version.
    • If the --version flag is set with the value active:
      • We consider the selected version to be the latest 'active' version.
    • If the --version flag is set with a numerical value (e.g. 1, 2, 3 etc):
      • We consider the selected version to be the given version.
    • If the --version flag is set with the value editable:
      • We consider the selected version to be the latest editable version.
      • If we find an active version before an editable version, then we return an error.
    • If the --version flag is not set (or an empty value is provided):
      • We consider the selected version to be the latest editable version.
      • If we find an active version before an editable version, then we return an error.
  • Once the version has been identified, if --autoclone is defined, we call a autoclone.Parse function.
    • This function checks if the version is 'active' or 'locked' and will clone it if necessary.
    • The autoclone.Parse function should only be called inside of commands where an 'editable' version is required.
    • The --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 either latest, active or 1. With --autoclone also specified this would cause a new editable service version 2 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 calling getLatestEditableVersion 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 value latest or editable 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 version 1 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 call getLatestEditableVersion, or --version=latest. Both would result in the editable version 1 being identified each time.

@Integralist Integralist added the enhancement New feature or request label Jun 4, 2021
@Integralist Integralist requested review from fgsch, a team and triblondon and removed request for a team June 4, 2021 14:36
@Integralist Integralist changed the title Integralist/overload version flag Overload --version flag to accept multiple values Jun 4, 2021
pkg/cmd/flags.go Show resolved Hide resolved
pkg/cmd/flags.go Outdated Show resolved Hide resolved
pkg/cmd/flags.go Outdated Show resolved Hide resolved
pkg/cmd/flags.go Outdated Show resolved Hide resolved
@fgsch
Copy link
Member

fgsch commented Jun 4, 2021

Looks good to me. Left a few minor comments, mostly around error messages.

@Integralist Integralist changed the title Overload --version flag to accept multiple values Define new --version and --autoclone flags Jun 4, 2021
@Integralist Integralist force-pushed the integralist/overload-version-flag branch from 1f05c7d to 88a7202 Compare June 4, 2021 17:15
Copy link
Member

@fgsch fgsch left a 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.

Copy link
Contributor

@triblondon triblondon left a 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?

pkg/cmd/flags.go Outdated Show resolved Hide resolved
pkg/cmd/flags.go Outdated Show resolved Hide resolved
pkg/cmd/flags.go Outdated Show resolved Hide resolved
pkg/cmd/flags.go Outdated Show resolved Hide resolved
pkg/cmd/flags.go Outdated Show resolved Hide resolved
pkg/cmd/flags.go Outdated Show resolved Hide resolved
@Integralist Integralist merged commit 24734d4 into main Jun 7, 2021
@Integralist Integralist deleted the integralist/overload-version-flag branch June 7, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operations on current and active version
3 participants