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

Document fields in spec to indicate when these were added. #215

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Jul 29, 2024

partial fix for #206

@bart0sh bart0sh force-pushed the PR018-document-spec-fields branch from e4b20f8 to 1e5f869 Compare July 29, 2024 15:13
@bart0sh bart0sh force-pushed the PR018-document-spec-fields branch from 1e5f869 to 61fe2c4 Compare July 29, 2024 15:15
@bart0sh
Copy link
Contributor Author

bart0sh commented Jul 29, 2024

@elezar @klihub PTAL

Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

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

For cases where fields were added, would it make sense to specify the version closer to the field name itself. e.g.: Annotations (string, OPTIONAL, v0.6.0)?

SPEC.md Show resolved Hide resolved
@bart0sh
Copy link
Contributor Author

bart0sh commented Aug 7, 2024

For cases where fields were added, would it make sense to specify the version closer to the field name itself. e.g.: Annotations (string, OPTIONAL, v0.6.0)?

I'd leave it as it is. "Added in v.6.0" sounds more informative to me.

@bart0sh bart0sh force-pushed the PR018-document-spec-fields branch from ca319de to 9c4f75c Compare August 8, 2024 04:12
@@ -19,6 +20,7 @@ type Spec struct {
type Device struct {
Name string `json:"name"`
// Annotations add meta information per device. Note these are CDI-specific and do not affect container metadata.
// Added in v0.6.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

One thought that I had was whether we wanted to add golang field tags so that we could parse these programatically? This could be done as a follow-up though.

@@ -10,6 +10,7 @@ type Spec struct {
Version string `json:"cdiVersion"`
Kind string `json:"kind"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to call out the allowed character change for the class in a docstring?

Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

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

This looks good.

I was wondering whether we wanted to add the version added as a Go struct tag, but this can be done as a follow-up after we've discussed it with others.

@elezar elezar merged commit 2752614 into cncf-tags:main Aug 8, 2024
7 checks passed
@elezar elezar mentioned this pull request Aug 8, 2024
18 tasks
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.

3 participants