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

What are we going to do to change the public data types? #638

Closed
arschles opened this issue Apr 3, 2017 · 13 comments
Closed

What are we going to do to change the public data types? #638

arschles opened this issue Apr 3, 2017 · 13 comments
Labels
Milestone

Comments

@arschles
Copy link
Contributor

arschles commented Apr 3, 2017

Data types should be cleaned up in preparation for our beta release (at which point we should keep them backward compatible. see https://github.com/kubernetes-incubator/service-catalog/wiki/Roadmap#010 for detail). Some ideas:

Note that this issue specifically targets the 0.0.7 release for changes to the public data types. We will create another issue for future milestones if necessary.

@pmorie
Copy link
Contributor

pmorie commented Apr 14, 2017

Moving this to the beta release milestone.

@arschles
Copy link
Contributor Author

arschles commented May 1, 2017

I'd like everyone to look at our data types (Broker, ServiceClass, Instance and Binding) between 5/1/2017 and 5/8/2017. Come to the SIG meeting on 5/8 with suggestions for changes. We'll decide on the final data types and implement necessary changes for the 0.1.0 release scheduled for 5/17/2017

@MHBauer
Copy link
Contributor

MHBauer commented May 1, 2017

For top-level names:

In the context of Service-Catalog and Service-Broker, I think the ServiceClass and ServicePlan names are the odd ones out and should be Service and Plan.

Within the context of kubernetes, Service could be problematic. I do not see any problem with Plan.

@duglin
Copy link
Contributor

duglin commented May 1, 2017

I'd like to better understand why we need a Checksum property. For Binding it should write-once so at worst we may need a boolean flag to indicate if we've created the binding on the broker yet or not. For Instances it seems like a boolean to indicate that we need to sync with the broker should be sufficient. Do we care which field changed? Especially when the checksum won't tell us which one, and right now only "plan" can change anyway. I'm not sure I see the value of the extra complexity of a Checksum when a boolean "dirty" type of flag will do.

(edit) @vaikas-google explained why we need to use a checksum instead of a dirty bit - so we can drop that part of the conversation.

@duglin
Copy link
Contributor

duglin commented May 1, 2017

Also, whether we keep checksum or not, it seems like that should be in the Status struct since its read-only to the user, right?

@MHBauer
Copy link
Contributor

MHBauer commented May 1, 2017

I think checksum to status makes sense.

@pmorie
Copy link
Contributor

pmorie commented May 5, 2017

I'd like to better understand why we need a Checksum property.

For posterity: we need the checksum field because we can't query the OSB API to see what the last successful state we reconciled to was. (See #582 and #573)

I agree - for now binding should be write-once. We could change to a boolean in the status if we wanted to. If we ever become able to update binding parameters, and we do not yet have a way to query the last values sent (see openservicebrokerapi/servicebroker#159), we would need to add the checksum back for bindings.

@MHBauer
Copy link
Contributor

MHBauer commented May 11, 2017

How about #297 ?

@pmorie
Copy link
Contributor

pmorie commented May 12, 2017

@MHBauer I just closed #297, but that would have been in scope if there was work to do :)

@pmorie pmorie added the api label May 12, 2017
@duglin
Copy link
Contributor

duglin commented May 15, 2017

opened #852 to deal with the checksum topics

@pmorie pmorie modified the milestones: 0.0.7, 0.1.0 May 15, 2017
@arschles arschles changed the title Clean up public data types What are we going to do to change the public data types? May 15, 2017
@arschles arschles modified the milestones: 0.1.0, 0.0.7 May 17, 2017
@duglin
Copy link
Contributor

duglin commented Jul 9, 2017

@arschles can we close this since it appears we have PRs and issues to track the issues mentioned?

@pmorie
Copy link
Contributor

pmorie commented Jul 31, 2017

I think #1080 supercedes this

@pmorie pmorie closed this as completed Jul 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants