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
Remove deprecated field and annotation #44900
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thockin
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
pkg/api/v1/types.go
Outdated
// Deprecated: Use serviceAccountName instead. | ||
// +k8s:conversion-gen=false | ||
// +optional | ||
DeprecatedServiceAccount string `json:"serviceAccount,omitempty" protobuf:"bytes,9,opt,name=serviceAccount"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
service account field LGTM, do we have a convention for marking dead protobuf ids we cannot use (or are we just counting on review to prevent someone from trying to fill in a gap left by a removed field in the future)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add a comment
I wondered that too. Google convention is to have a comment at the end of
the struct, but this is all auto-gen. Do we need to leave the field here
forever as a placeholder?
…On Mon, Apr 24, 2017 at 9:45 PM, Jordan Liggitt ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/api/v1/types.go
<#44900 (comment)>
:
> @@ -2342,11 +2342,6 @@ type PodSpec struct {
// More info: http://releases.k8s.io/HEAD/docs/design/service_accounts.md
// +optional
ServiceAccountName string `json:"serviceAccountName,omitempty" protobuf:"bytes,8,opt,name=serviceAccountName"`
- // DeprecatedServiceAccount is a depreciated alias for ServiceAccountName.
- // Deprecated: Use serviceAccountName instead.
- // +k8s:conversion-gen=false
- // +optional
- DeprecatedServiceAccount string `json:"serviceAccount,omitempty" protobuf:"bytes,9,opt,name=serviceAccount"`
this LGTM, do we have a convention for marking dead protobuf ids we cannot
use (or are we just counting on review to prevent someone from trying to
fill in a gap left by a removed field in the future)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#44900 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVHmMi9RuYdAP2jVNUzk-ptSaoYPgks5rzXp_gaJpZM4NG_nh>
.
|
need to sweep examples/docs (at least in-tree) for use of |
Never discount an enterprising (or confused) developer that manually assigns a protobuf id, not knowing the generation would do it for them.
Not live, but maybe commented out in a "removed fields" section? @smarterclayton, thoughts? |
Have we deprecated Also, it's a high-risk change: a service that was locked down will suddenly be opened up to everyone I believe. I'm not even sure where we do actually deprecate things - release notes? |
The allocator is not supposed to hole fill. Let's go ahead and add a
comment that indicates the deprecated ID and I'll make gengo respect it and
error
// +k8s:protobuf-deprecated=fieldName,ID
Maybe?
On Apr 25, 2017, at 12:50 AM, Jordan Liggitt <notifications@github.com> wrote:
I wondered that too. Google convention is to have a comment at the end of
the struct, but this is all auto-gen.
Never discount an enterprising (or confused) developer that manually
assigns a protobuf id, not knowing the generation would do it for them.
Do we need to leave the field here forever as a placeholder?
Not live, but maybe commented out in a "removed fields" section?
@smarterclayton <https://github.com/smarterclayton>, thoughts?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#44900 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p4fIEN9uMFf5gY7V0maurrrEak-Bks5rzXuHgaJpZM4NG_nh>
.
|
would be good to get this in at the beginning of 1.8 |
fixes: kubernetes/kubectl#23 |
aaf46dd
to
92d52ff
Compare
03ed764
to
5347762
Compare
3e8298b
to
948d09b
Compare
4f87776
to
76363d9
Compare
@thockin: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
This is ready for review, but many of the earlier commits are already out as PRs of their own. |
removal of the field still LGTM, quick search of |
76363d9
to
d64b4c2
Compare
@thockin PR needs rebase |
This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review. You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days |
Remove some long-deprecated or expired fields and annotations.