-
Notifications
You must be signed in to change notification settings - Fork 468
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
Returning NamedAddress type and marking it deprecated #1252
Returning NamedAddress type and marking it deprecated #1252
Conversation
Although relatively tiny in scope, this undoes part of a previous change, so want to make sure there's some consensus here. /hold |
eafa787
to
6b19b11
Compare
I'm not necessarily strongly for or against adding this back. For posterity how many implementations were wanting this to be added back and what was the reasoning if other than "need more time to update"? |
6b19b11
to
928baa6
Compare
My primary argument here is that removing it would break our backwards compatibility guarantees. In general we've said we can loosen validation or add new valid values, but if something was at some point considered valid, we will not prevent it from being set in the future unless it was very clearly a bug in our validation. In this case it represented a defined and valid value, so I don't think we can get away with removing it. We have removed suggested reasons for status from the API, but I think the important distinction there is that we still allow implementations to populate those reasons, we just don't define them as suggested reasons anymore. In upstream Kubernetes we've sometimes tweaked defaults between API versions, but I don't think we've ever tightened validation. |
I understand. I'm going to approve as I understand and can appreciate your reasoning, and I don't have a reason to block. I do sometimes wonder if it would be better to take a harder and faster approach at these earlier maturity states of a project, but this still ends up being a pretty minor thing in either case. |
/approve |
Yeah I generally agree with that approach, but unfortunately my understanding of k8s API guidelines is that although we could make a breaking change in alpha, that would need to be followed by another alpha API version, not a beta API version. I don't think this is significant enough of a change to merit another alpha API release. |
Discussed this at today's community meeting and the change had broad consensus. As mentioned above, this change is to preserve backwards compatibility, the type should still be considered deprecated. Removing the hold on this. /hold cancel |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nathancoleman, robscott, shaneutt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it:
This returns our NamedAddress type and marks it deprecated. Unfortunately I think we went a step too far when we removed it altogether. Although we've left room to add new values to enums in new minor versions, we have not provided any precedent for removing supported enum values. Unfortunately I think removing "NamedAddress" from the list of supported values would make it so HTTPRoutes that were valid in v0.4.x would not be valid in v0.5.x, which is something we should strive to avoid.
I think the best we can do here is continue to allow the value to be set but document that it has been deprecated in favor of domain prefixed strings. Hopefully this approach makes sense.
Does this PR introduce a user-facing change?: