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.

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

CSI: add missing plugin capabilities to api response #12108

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Conversation

tgross
Copy link
Member

@tgross tgross commented Feb 23, 2022

Detection of the full set of plugin capabilities was added in Nomad
1.1 for the volume creation workflow, but these were not added to the
API response for plugins.

Blocks showing caps in plugin status -verbose #10221
Nice-to-have for implementing topology in #7669

@tgross tgross added this to the 1.3.0 milestone Feb 23, 2022
@tgross tgross added the theme/api HTTP API and SDK issues label Feb 23, 2022
Detection of the full set of plugin capabilities was added in Nomad
1.1 for the volume creation workflow, but these were not added to the
API response for plugins.
Copy link
Contributor

@DerekStrickland DerekStrickland left a comment

Choose a reason for hiding this comment

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

My one comment would be that some field names are camel cased versions of the full setting name and some are partial. For example, if you had SETTING_NAME sometimes the field is SupportsSettingName and sometimes it's SupportsSetting. Not a blocker, but I think it is less ambiguous to use the full setting name, and also helps guard against name collision or prefix reuse upstream that we may not have control over. I realize it's verbose, but that my 2 cents.

@tgross
Copy link
Member Author

tgross commented Feb 23, 2022

My one comment would be that some field names are camel cased versions of the full setting name and some are partial.

I don't disagree and in retrospect I think I probably would have returned a list of these instead of bools anyways. Way-back-when we'd decided we were definitely not going to support create volume, but then came back months later and did it anyways, but by then we had the bool fields and it got pretty gory. 🤣

But in any case, here I'm actually matching the field names from the existing structs version of the same fields (ex structs.CSIContollerInfo), and we don't want to differ between them so that serialization works without having to write it all by hand.

@tgross tgross merged commit 822285f into main Feb 23, 2022
@tgross tgross deleted the csi-api-plugin-caps branch February 23, 2022 20:22
tgross added a commit that referenced this pull request Feb 24, 2022
In PR #12108 we added missing fields to the plugin response, but we
didn't include the manual serialization steps that we need until
issue #10470 is resolved.
tgross added a commit that referenced this pull request Feb 24, 2022
In PR #12108 we added missing fields to the plugin response, but we
didn't include the manual serialization steps that we need until
issue #10470 is resolved.
@lgfa29 lgfa29 added backport/1.1.x backport to 1.1.x release line backport/1.2.x backport to 1.1.x release line labels Apr 19, 2022
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.1.x backport to 1.1.x release line backport/1.2.x backport to 1.1.x release line theme/api HTTP API and SDK issues theme/storage type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants