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

RFE: Make the toolbox idempotent #81

Closed
nmasse-itix opened this issue Dec 14, 2018 · 11 comments · Fixed by #164
Closed

RFE: Make the toolbox idempotent #81

nmasse-itix opened this issue Dec 14, 2018 · 11 comments · Fixed by #164

Comments

@nmasse-itix
Copy link
Contributor

nmasse-itix commented Dec 14, 2018

The 3scale toolbox is being improved to become the recommended way to configure and deploy services in 3scale. This fits in a broader approach of Continuous Delivery that has been started by several customers / communities (I personally maintain the threescale-cicd ansible role. Those approaches are using different means to drive the 3scale admin portal, some are using the 3scale-cli other (like the threescale-cicd role) are using the 3scale REST APIs.

If we want to make the 3scale_toolbox the de facto standard for interacting with 3scale, we need it to be usable in a CI/CD approach.

At a glance, here are the features I expect from the 3scale toolbox :

Idempotency

When continuous delivering an API, the delivery pipeline needs to create or update the 3scale service definition in addition to each linked object.

Forcing the user to specify if the service needs to be created or updated leads to a lots of code in the pipeline (list existing services, create is missing, update if existing).

Proposition

The 3scale_toolbox does not enforce the user to choose between create or update operations. Instead, if the object exists, it updates it, otherwise it creates it.

Example

3scale copy service -t my-prod-api -s dev -d prod my-dev-api

=> If the service exists, it is updated
=> Otherwise it is created

This feature needs to be in the toolbox's core so that every plugin can leverage this feature.

@eguzki
Copy link
Member

eguzki commented Dec 19, 2018

In the beginning I was not convinced about this feature of offering a way for create or update out of the box. After a productive discussion with @mikz, I implemented OpenAPI spec importing command this way. I am now quite satisfied with the resulting behavior. It's idempotent and CLI user does not deal with service ID, just Service system_name.

Indeed, I now think this is the pattern to go with copy service command. It would merge 3scale copy service and 3scale update service into a single command. Breaking backwards compatibility though. To be taken into account as well.

@eguzki
Copy link
Member

eguzki commented Mar 5, 2019

Making 3scale copy service idempotent and working with system_name and not id (#82), breaks backwards compatibility.

One proposition would be to deprecate 3scale copy service and 3scale update service and instead, provide sync command:

$ 3scale sync service --help
NAME
    service - Synchronize services

USAGE
    3scale sync service [opts] <src> <dst>  <source-system-name>

DESCRIPTION
    Will sync source and destination service. If destination service does not exist, it creates it.
    
OPTIONS
    -t --target_system_name=<value>      Target system name. Defaults to source system name

src and dst are required parameters. They will be positional parameters instead of options that are considered required as in copy or update

WDYT @mikz @nmasse-itix, should we break backwards compatibility and change 3scale copy service command like this sync or implement new sync command and deprecate copy and update?

Maybe sync is not most appropriate action command. I am open to wording proposals. @thomasmaas might have something to say about command naming.

@mikz
Copy link
Contributor

mikz commented Mar 5, 2019

@eguzki yep, the name is quite unfortunate, but that can be worked out.

How working with system_name breaks backwards compatibility? It is extending the functionality, but not breaking anything no?

What is the difference between copy and update, that sync would unify?
I think it is important to start with use cases, ask questions and design the commands accordingly.

For example:

  • copy service for testing into staging account (new every time?, what to do with differences, etc.)
  • update production from some other account (like staging)
    ...

And when designing new commands I'd seriously consider not having the verb first, but rather the object (eg. 3scale service copy).

@eguzki
Copy link
Member

eguzki commented Mar 5, 2019

3scale copy service requires source service service_id. Moreover, it will try to create a service, no matter what. To update existing service, the available command is 3scale update service.

The idea behind the improvement is twofold:

  • work with ''system_name` not id's
  • Create service if not exists, update then. Be idempotent.

WRT, 3scale service copy or 3scale copy service I do not have strong opinion about it.
We could implement 3scale service copy and leave 3scale copy service as old deprecated command

@thomasmaas
Copy link
Member

thomasmaas commented Mar 7, 2019

I agree with @mikz that object, verb is a better order.

Naming-wise, sync is indeed confusing. Alternatives: service create_or_update, service copy_or_update.

I was playing with the idea that copy, update and import could all be combined in one: the source can be a service or a specification. In the end the goal is to create or update a service. @nmasse-itix & @mikz what do you think?

@mikz
Copy link
Contributor

mikz commented Mar 7, 2019

@eguzki but how is requiring service_id or service_system_name a breaking change?
We just start accepting a different format of the identifier. The old ways of using it should still keep working, correct?

We could start unifying update and copy with flags, just add a flag --create to copy and allow it creating a service. Then there would be no difference between new suggested command and copy ?

@nmasse-itix
Copy link
Contributor Author

I would prefer something like this:

with named parameters:

3scale service copy --from 3scale-dev/my-service --to 3scale-prod/my-service

or with positional parameters

3scale service copy 3scale-dev/my-service 3scale-prod/my-service

syntax for --from and --to is {remote_name}/{service_name}

If the target service is already there, it is created otherwise just updated and this should be the default behavior.

@mikz I would like the 3scale toolbox to be as idempotent as possible and do not require special options to differentiate between create and update. Otherwise, it clutters the CI/CD pipeline with:

if (service is missing) {
  3scale service create
} else {
  3scale service update
}

See the tasks/steps folder for concrete exemple of this.

@mikz
Copy link
Contributor

mikz commented Mar 7, 2019

@nmasse-itix I understand. That is still doable without any breaking change with a single flag to update to allow creating a service when it does not exist.

3scale copy service --allow-create --source=some --destination=other

Sure, we eventually want better UX, but for a start to explore the ideas, this is enough IMO.

@eguzki
Copy link
Member

eguzki commented Mar 7, 2019

@mikz

I find confusing something like this

3scale copy service -s source -d target <service_id_or_system_name>

And for implementation details, toolbox first tries to find service with that system name, if it fails, tries with service_id?

I like the design of @nmasse-itix it is very clear

scale service copy 3scale-dev/my-service-system-name 3scale-prod[/my-service-system-name]

We can modify copy command to enable "updating" when exists and deprecate update command. Still the thing of <service_id_or_system_name> is confusing top me

@mikz
Copy link
Contributor

mikz commented Mar 7, 2019

@eguzki implementation wise it is easy, id is numerical and system_name contains other characters, so just try to find it by ID if it is numerical and fall back to system_name otherwise. But eventually, this should be provided by the porta API and transparent for the toolbox.

I agree that the design suggested in
#81 (comment) is very clear and would have good UX. For now, we can iterate on what we have and when we gather enough requirements/use cases we can implement a new command that works for all of them.

@eguzki
Copy link
Member

eguzki commented Mar 7, 2019

Ok, I will proceed changing copy service command.

3scale copy service -s source -d target [-t target-system-name] <service_id_or_system_name>
  • Will be idempotent. If target service does not exist, it will be created, otherwise updated.
  • target system name will be source system name, but can be overridden with -t option param.

This somehow deprecates 3scale update service since all the functionality will be covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants