Skip to content

v1 is specified in apiVersion field#4127

Closed
madorn wants to merge 1 commit intokubernetes:masterfrom
madorn:md
Closed

v1 is specified in apiVersion field#4127
madorn wants to merge 1 commit intokubernetes:masterfrom
madorn:md

Conversation

@madorn
Copy link
Contributor

@madorn madorn commented Jun 17, 2017 •
edited by k8s-reviewable
Loading

The use of not here may be confusing to newcomers.

Is the intention to suggest the apiVersion field defaults to v1 unless otherwise specified?

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
For 1.7 Features: set Milestone to 1.7 and Base Branch to release-1.7
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

NOTE: Please check the “Allow edits from maintainers” box below to allow
reviewers fix problems on your patch and speed up the review process.
Please delete this note before submitting the pull request.

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 17, 2017
@chenopis chenopis requested a review from ahmetb June 19, 2017 23:25
@ahmetb
Copy link
Member

ahmetb commented Jun 20, 2017 •
edited
Loading

I am not equipped to answer this question myself (I am confused too) so I will defer the tech review to someone else. cc: @kubernetes/sig-api-machinery-pr-reviews

@chenopis chenopis requested a review from smarterclayton June 22, 2017 21:00
sttts
sttts reviewed Jul 3, 2017
View reviewed changes
docs/concepts/overview/kubernetes-api.md

1. the "core" (oftentimes called "legacy", due to not having explicit group name) group, which is at
REST path `/api/v1` and is not specified as part of the `apiVersion` field, e.g. `apiVersion: v1`.
REST path `/api/v1` and is specified as part of the `apiVersion` field, e.g. `apiVersion: v1`.
Copy link
Contributor

@sttts sttts Jul 3, 2017

Choose a reason for hiding this comment

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

it is meant that the apiVersion value is not /v1 (i.e. empty group name), but v1.

Copy link
Member

@ahmetb ahmetb Aug 5, 2017

Choose a reason for hiding this comment

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

@madorn 👋 hello there, do you have time to address the PR feedback?

Copy link
Contributor Author

@madorn madorn Sep 1, 2017

Choose a reason for hiding this comment

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

@ahmetb @sttts thanks for clarification. it would be more clear to newcomers if we remove the use of not here and just suggest using apiVersion: v1.

Copy link
Contributor

@sttts sttts Sep 1, 2017

Choose a reason for hiding this comment

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

maybe we should reformulate the sentence and avoid "be or not be specified". Seems to be hard to understand.

Copy link
Contributor

@chenopis chenopis Sep 29, 2017

Choose a reason for hiding this comment

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

So, should the sentence be:

The core group, often referred to as legacy, is at REST path /api/v1 and can be accessed using apiVersion: v1.

Copy link
Contributor

@sttts sttts Oct 4, 2017

Choose a reason for hiding this comment

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

"can be accessed using ..." still sounds odd.

How about

The core group, often referred to as the "legacy group", is at the REST path /api/v1 and uses `apiVersion: v1`.

E.g. the same wording as in the next paragraph.

@chenopis
Copy link
Contributor

chenopis commented Sep 29, 2017

@madorn @sttts Is my suggestion for rewording the sentence sufficient?

@steveperry-53
Copy link
Contributor

steveperry-53 commented Oct 5, 2017

Ping @madorn @sttts

@xiangpengzhao xiangpengzhao added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 19, 2017
@steveperry-53
Copy link
Contributor

steveperry-53 commented Oct 19, 2017

Sorry, I don't agree with this change. The group name "core" is not specified as part of the apiVersion field. So this sentence seems incorrect to me:

the “core” ... group, which is at REST path /api/v1 and is specified as part of the apiVersion field, e.g. apiVersion: v1.

@sttts
Copy link
Contributor

sttts commented Oct 19, 2017

@steveperry-53 compare my comment #4127 (comment). The point of the PR is to clarify exactly this. So the issue is still open and correct variants are proposed in the review comments. We are just waiting for the PR being updated.

@sttts sttts reopened this Oct 19, 2017
@steveperry-53
Copy link
Contributor

steveperry-53 commented Oct 19, 2017

@madorn @stts @cheopis @ahmetb
Now that this PR has been reopened, let's move forward. @madorn Please make the suggested change.

I'm OK with this:
The core group, often referred to as the "legacy group", is at the REST path /api/v1 and uses apiVersion: v1.

@ahmetb
Copy link
Member

ahmetb commented Oct 19, 2017

I think the best course of action is to close this PR and open another.

@steveperry-53 steveperry-53 mentioned this pull request Oct 23, 2017
Merged
@steveperry-53
Copy link
Contributor

steveperry-53 commented Oct 23, 2017

In the interest of moving this forward, I'm going to create a duplicate.
Duplicate of #6024.

@ahmetb ahmetb removed their request for review November 10, 2017 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

3 more reviewers

@ahmetb ahmetb ahmetb left review comments

@sttts sttts sttts left review comments

@chenopis chenopis chenopis left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@chenopis chenopis

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@madorn @ahmetb @chenopis @steveperry-53 @sttts @zacharysarah @xiangpengzhao @k8s-ci-robot