Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Rename our resources to have ServiceCatalog prefix #1054

Merged
merged 2 commits into from Jul 24, 2017

Conversation

duglin
Copy link
Contributor

@duglin duglin commented Jul 20, 2017

Please check to see if I went too far on the resources I changed.
I tried to limit it to just the ones that might be visible as top-level resources.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 20, 2017
@@ -148,26 +149,27 @@ func TestInstallTypesErrors(t *testing.T) {
//make sure we don't poll on resource that was failed on install
func TestInstallTypesPolling(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arschles I don't understand this test. I don't see how it ever worked, but apparently it must. The part that confuses me is the use of getCallCount and comparing it to len(thirdPartyResources). That just seems really odd since we're going to Get() each resource at least once during the install (once before we install and then at least once after) and we never reset the getCallCount so it seems like it should always generate an error 1/2 way thru the installation of the resources.

@pmorie
Copy link
Contributor

pmorie commented Jul 20, 2017

@smarterclayton will this pass API review?

Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

move the generated code deletions out of the first commit.

@duglin
Copy link
Contributor Author

duglin commented Jul 21, 2017

@MHBauer I've been trying but to be honest I don't know how. "make purge-generated" doesn't do it like I would expect.

@duglin duglin force-pushed the rename branch 4 times, most recently from 14c2135 to 7123d6b Compare July 21, 2017 18:11
@duglin duglin dismissed MHBauer’s stale review July 21, 2017 18:13

they're now in a separate commit

@duglin duglin force-pushed the rename branch 5 times, most recently from a2ea3f7 to da12fa0 Compare July 22, 2017 02:09
@duglin
Copy link
Contributor Author

duglin commented Jul 22, 2017

@kibbles-n-bytes I can't seem to see where the error in the travis output is - do you see it?

@duglin duglin changed the title [WIP] Rename our resources to have ServiceCatalog prefix Rename our resources to have ServiceCatalog prefix Jul 22, 2017
@duglin duglin force-pushed the rename branch 7 times, most recently from 41e42a5 to 2ccbaa9 Compare July 22, 2017 11:54
@duglin
Copy link
Contributor Author

duglin commented Jul 22, 2017

ok this one is ready for review. I'd appreciate a quick one because the potential for conflicts/rebasing is pretty darn high.

@duglin
Copy link
Contributor Author

duglin commented Jul 24, 2017

rebased now that we're on kube 1.7 api machinery - wasn't nearly as bad as I expected.

Doug Davis added 2 commits July 24, 2017 08:17
Signed-off-by: Doug Davis <dug@us.ibm.com>
Signed-off-by: Doug Davis <dug@us.ibm.com>
@smarterclayton
Copy link

I don't see a problem with the prefix. For some of them, I think it could be shortened to just Service and that would be acceptable.

For instance, I don't know that anyone in the Kubernetes community could argue that the idea of a "ServiceClass" (like PriorityClass, StorageClass, etc) "belongs" to the service catalog work. And likewise, the idea of Binding a service class to a user also belongs this this work. While I could maybe imagine a ServiceInstance existing in another API group, I think it's more likely to see things like "ProxyService" or "MirroredService".

So I think if this SIG wanted to, it could legitimately claim ownership of ServiceInstance, ServiceClass, and ServiceBinding. To a user, that may also be clearer and less to type. The chance of conflict seems low enough that you can make that leap.

Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

Good job fixing the docs and stuff. I didn't review all of it, but most of the non-code stuff and types.go and some of the apimachinery relations.

compiles and runs fine.

LGTM

@MHBauer MHBauer added the LGTM1 label Jul 24, 2017
@MHBauer
Copy link
Contributor

MHBauer commented Jul 24, 2017

We're going to want a release shortly after this so that people don't pull an image that doesn't work as it doesn't match the code.

@pmorie
Copy link
Contributor

pmorie commented Jul 24, 2017

I also think ServiceInstance is probably preferable to ServiceCatalogInstance, but I won't block this PR on that.

@pmorie pmorie removed the LGTM1 label Jul 24, 2017
@pmorie
Copy link
Contributor

pmorie commented Jul 24, 2017

Oops, total fail re: Label

@pmorie
Copy link
Contributor

pmorie commented Jul 24, 2017

Looking forward to finalizing the names of these resources

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants