Skip to content

Drop GetSupportedVersions in favor of versioned proto package #202

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

Conversation

jdef
Copy link
Member

@jdef jdef commented Feb 22, 2018

As per the CSI discussion today regarding versioning:

@jdef jdef added this to the v0.2 milestone Feb 22, 2018
@@ -568,15 +499,13 @@ Such actions MAY include, but SHALL NOT be limited to, the following:
* Notifying the plugin supervisor.

The Plugin MAY verify that it has the right configurations, devices, dependencies and drivers in order to run and return a success if the validation succeeds.
The CO MAY invoke this RPC at any time after version negotiation has been completed (see `GetSupportedVersions`).
The CO MAY invoke this RPC at any time.
Copy link
Member Author

Choose a reason for hiding this comment

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

If we decide to add a Versioned service later (and add back GetSupportedVersions) there will be ordering dependencies here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's ok...we would just need to add the version with a different position id. In other words, the following is totally valid:

message ControllerUnpublishVolumeRequest {
  Version version = 4;
  string volume_id = 1;
  ...
}

@jdef jdef requested review from jieyu and saad-ali February 22, 2018 22:55
@jdef
Copy link
Member Author

jdef commented Feb 22, 2018

@jieyu @saad-ali @cpuguy83 @julian-hj PTAL

Copy link
Member

@jieyu jieyu left a comment

Choose a reason for hiding this comment

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

This LGTM!

Copy link
Contributor

@julian-hj julian-hj left a comment

Choose a reason for hiding this comment

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

LGTM

@saad-ali
Copy link
Member

the "v1" in the proto package name may or may not correspond to the major spec version; this PR does not decide that

Can we call it something else then, build1, iteration1, something to make it more obvious that this is not the something as the version of spec?

@cpuguy83
Copy link
Contributor

v1-dev which gets bumped to v1 on release?
I'm not even really a big fan of that...
As long as it's clearly documented that this is for package namespacing and not signifying a formal release process, I think it's fine.

@saad-ali
Copy link
Member

I guess what will get confusing is, if I want to support v1.2 and v1.3 how do I know which of these internal versions correspond to that external version. If we just align our external release versions with the internal version, that would be trivial.

@jieyu
Copy link
Member

jieyu commented Feb 23, 2018

If you guys are concerned with using v1 at this moment, i'd suggest use v0 for this release, and try to jump to v1 and 1.0 in the next release.

@cpuguy83
Copy link
Contributor

The package namespacing is very much tied to the release, but there is no need to bump the package namespacing to support new features, which is why it's tied to vMAJOR.

@saad-ali
Copy link
Member

If you guys are concerned with using v1 at this moment, i'd suggest use v0 for this release, and try to jump to v1 and 1.0 in the next release.

I like this idea. Tying the package name to the Major release makes sense. We can call this v0 and push to get to v1 sooner.

@jieyu
Copy link
Member

jieyu commented Feb 23, 2018

@cpuguy83 @julian-hj are you guys ok with s/v1/v0/ in this PR. If yes, i'll prepare a commit since @jdef is out today

@cpuguy83
Copy link
Contributor

I think that works.

@julian-hj
Copy link
Contributor

julian-hj commented Feb 23, 2018 via email

@jieyu
Copy link
Member

jieyu commented Feb 23, 2018

Resolved by #204

@jieyu jieyu closed this Feb 23, 2018
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.

Consider adding vMAJOR version (e.g. csi.v1.) to either the API bindings or package names
5 participants