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

Rename cloud_type from GCE to GCP, clarify throughout #58

Merged
merged 1 commit into from
May 5, 2020

Conversation

eolix
Copy link
Contributor

@eolix eolix commented May 5, 2020

cloud_type was renamed from gce to gcp to maintain consistency with the aws/ec2 documentation and usage throughout the playbook.

It introduces one breaking change for playbooks that run on GCP, as the cloud_type variable would have to be renamed.

@dseeley-sky
Copy link
Contributor

Theoretically, application playbooks should not have a dependency upon clusterverse's cluster_vars, so this should not be a breaking change...

@eolix eolix requested a review from flfernandez May 5, 2020 09:49
Copy link
Contributor

@antoineserrano antoineserrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - confirmed not being a breaking change.

@dseeley-sky
Copy link
Contributor

Contrary to my earlier statement, the change will require all application playbooks that use clusterverse with GCP to change the cloud_type in their cluster_vars, (as Patrik mentioned initially). Is it worth forcing everyone else to change their application playbooks for what is essentially a cosmetic change?

@dseeley-sky
Copy link
Contributor

Contrary to my earlier statement, the change will require all application playbooks that use clusterverse with GCP to change the cloud_type in their cluster_vars, (as Patrik mentioned initially). Is it worth forcing everyone else to change their application playbooks for what is essentially a cosmetic change?

Maybe there are few enough users within GCP that this has a fairly small impact right now?

@eolix
Copy link
Contributor Author

eolix commented May 5, 2020

It was my thought as well as this is, by all means, a cosmetic change. However, with a wider GCP adoption in the future this might get flagged nonetheless.

The expected changes from Clusterverse/GCP user point-of-view is a one-liner:

cluster_vars:
  type: gcp

... so I doubt it will create much backlash. What are your thoughts @mikekrasn and @flfernandez ?

@dseeley-sky dseeley-sky requested a review from leslieonline27 May 5, 2020 11:03
@mikekrasn
Copy link
Contributor

The change is indeed minimal from a user perspective. I personally agree with it.

@leslieonline27
Copy link
Contributor

Currently for Persistence there is no worries changing this, but I know that Content Discovery used clusterverse for their Couchbase build out in GCP towards the end of last year. I think we'd need to sync with them on this change, but as Patrik's said it's a small modification of the cloud type value so I don't think it would be an issue

@eolix
Copy link
Contributor Author

eolix commented May 5, 2020

@sky-amoncadot : I don't know who picked up the Couchbase work in CD, as KL has left the department, but from your perspective would this be a major impacting change?

@eolix
Copy link
Contributor Author

eolix commented May 5, 2020

Confirmed by @PreeyanP this isn't an issue for CD

@antoineserrano antoineserrano merged commit 77612cd into master May 5, 2020
@eolix eolix deleted the cleanup-gcp-gce branch May 6, 2020 08:55
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 this pull request may close these issues.

6 participants