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.

Sign up for GitHub

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

Jump to bottom

Add experimental Alpha API support #417

Merged
merged 1 commit into from
Mar 5, 2020
Merged

Add experimental Alpha API support #417

merged 1 commit into from
Mar 5, 2020

Conversation

xing-yang
Copy link
Contributor

@xing-yang xing-yang commented Feb 24, 2020 •
edited
Loading

Submit the following commit from #365 to provide Alpha API support:

4cf1497

Fixes #355

@xing-yang xing-yang mentioned this pull request Feb 24, 2020
Merged
@jdef jdef mentioned this pull request Feb 24, 2020
Open
@jdef
Copy link
Member

jdef commented Feb 24, 2020

@saad-ali @julian-hj @jieyu PTAL

jieyu
jieyu reviewed Feb 27, 2020
View reviewed changes
spec.md Outdated
@@ -601,10 +636,22 @@ message PluginCapability {
Type type = 1;
}

message AlphaFeature {
enum Type {
UNKNOWN = 0;
Copy link
Member

@jieyu jieyu Feb 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Am I understand correctly that for each new alpha feature capability, a new enum will be added here? If that's the case, another option would be to mark the corresponding enum as alpha, like

message Service {
  enum Type {
    UNKNOWN = 0;
    CONTROLLER_SERVICE = 1;
    NEW_ALPHA_SERVICE=3 [(alpha_enum) = true];
  }
  Type type;
}

I actually don't see a huge value adding this AlphaFeature message here.

Copy link
Member

Choose a reason for hiding this comment

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

you mean alpha_enum_value - right?

PluginCapability lets you pick a Service capability, or a VolumeExpansion capability. It's not clear to me whether all alpha features are a good fit for either. At the same time, PluginCapability.type is a oneof which has pretty strict rules re: backwards compat. If possible it would be nice to make that oneof strictly additive, which my mind ruled out alpha things. So I came up with AlphaFeature as a dumb bucket for things. And if we want to be consistent, people can (should?) just put their alpha features in there while they're iterating .. before things are promoted to stable.

I think that I also remember that someone objected to polluting Service/PluginCapability enums w/ alpha things - and AlphaFeature also seemed to check that box too.

That said, nothing stops someone from dumping an alpha-tagged enum value in the Service bucket. But I tried to avoid that in my examples (see #365) for the sake of consistency.

Also, it might be worth adding a NOTE to the oneof fields that they should not be polluted w/ alpha things. Docs...

Copy link
Member

Choose a reason for hiding this comment

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

At the same time, PluginCapability.type is a oneof which has pretty strict rules re: backwards compat. If possible it would be nice to make that oneof strictly additive, which my mind ruled out alpha things.

I think we can make it strictly additive, if an alpha feature does not make to the end, we can mark it as deprecated.

I think that bag of holding AlphaFeatures makes it hard to GA some fields/enums/... eventually as we'll have to remove it from AlphaFeatures and then add it to Services (or others).

Copy link
Member

@jdef jdef Mar 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think that was one of the objections raised, that people didn't want to carry around deprecated or outdated alpha API fields / types / values. we talked about maybe using "reserved" for things like this - as a way to declutter the API w/ respect to outdated names, but we'd still have the baggage of the old identifiers (field/value numbers) hanging around.

the way things have been going, i don't think the above concern is going to be a big problem and i'm happy to remove the AlphaFeature bag if others agree that this is the direction we should go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@xing-yang
spec: added alpha_xxx options
6e109ec 
Enumerations, fields, messages, methods, and services support
`alpha_xxx` designations, indicating that they are part of an
experimental feature that may never evolve to "stable" status.
@xing-yang
Copy link
Contributor Author

Removed AlphaFeature bucket.

@jieyu @jdef @saad-ali @julian-hj Please take a look.

@jdef
Copy link
Member

jdef commented Mar 5, 2020 via email

@saad-ali
Copy link
Member

saad-ali commented Mar 5, 2020

/lgtm

@saad-ali
Copy link
Member

saad-ali commented Mar 5, 2020

Will let @jieyu and @julian-hj LGTM before merge.

@julian-hj
Copy link
Contributor

julian-hj commented Mar 5, 2020 via email

@jieyu
Copy link
Member

jieyu commented Mar 5, 2020

@saad-ali i think @xing-yang included this patch in #415

@jdef
Copy link
Member

jdef commented Mar 5, 2020 via email

@xing-yang
Copy link
Contributor Author

I'm fine either way.

@jieyu
Copy link
Member

jieyu commented Mar 5, 2020

OK, let's merge this first then. @xing-yang

@jieyu jieyu merged commit e4404c3 into container-storage-interface:master Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdef jdef jdef approved these changes

@jieyu jieyu jieyu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Alpha Beta GA designation for new capabilities
5 participants
@xing-yang @jdef @saad-ali @julian-hj @jieyu