openshift / cluster-operator

52 stars 35 forks source link

Remove ClusterID field from MachineSetSpec #298

Closed twiest closed 6 years ago

twiest commented 6 years ago
twiest commented 6 years ago

/retest

twiest commented 6 years ago

@staebler Hey, this is my first shot at removing ClusterID from MachineSetSpec. It is fully functional, but before I ask for a review, I want to make sure this is what you were thinking.

Is this what you were looking for?

staebler commented 6 years ago

@twiest It looks good to me. The only question that I would raise is whether ClusterID in ClusterDeploymentSpec should be renamed to ClusterName. However, even if that is something worthwhile to do, it can be done as a separate PR.

twiest commented 6 years ago

@staebler I'm fine making that change (either as part of this PR or a separate PR).

@dgoodwin What are your thoughts on that change and whether it should be part of this PR or another PR?

dgoodwin commented 6 years ago

Totally up to you

twiest commented 6 years ago

@staebler @dgoodwin Ok, I think I'd prefer to merge this PR and I'll start a new PR with the new changes.

Can I get a lgtm please?

dgoodwin commented 6 years ago

/lgtm

dgoodwin commented 6 years ago

/test e2e