Bring back removed Mesh config entries.#663
Conversation
istio-testing
added
the
do-not-merge/work-in-progress
googlebot
added
the
cla: yes
| AuthPolicy auth_policy = 10 [deprecated=true]; | ||
|
|
||
| reserved 11; | ||
| // $hide_from_docs |
There was a problem hiding this comment.
I don't think we want $hide_from_docs. Just mark the field as deprecated and it will be rendered appropriately by the doc infra.
There was a problem hiding this comment.
We don't want to render this.. as this is not implemented at all. Unless you mean marking something as deprecated will also hide it from the docs.
There was a problem hiding this comment.
Marking deprecated will indicate that is deprecated, but otherwise it is visible.
What do you mean "not implemented at all"? Was the field available in 1.0 (I thought it was).
The higher-order decision bit is, whether there are config maps that have a value for this field or not. If we can say we won't have for sure, then we can simply remove it. If not (i.e. there was a default value put when config was being generated), then we can't remove it.
(rendering is a separate thing, I'll let you guys come to an agreement and do the what is agreed on ).
|
/lgtm |
ozevren
changed the title
ozevren
removed
the
do-not-merge/work-in-progress
| // requested destination. | ||
| // Users are strongly encouraged to use ServiceEntries to explicitly declare any external dependencies, | ||
| // instead of using allow_any. | ||
| OutboundTrafficPolicy outbound_traffic_policy = 17 [deprecated=true]; |
There was a problem hiding this comment.
This was never implemented from the beginning. Do we have to keep it/bring it back?
There was a problem hiding this comment.
Did we actually create any mesh config files with a value for this field? If so, yes.
There was a problem hiding this comment.
Nope. As I said not even in docs or helm.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: geeknoid The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
We decided at TOC that deprecated fields should be visible in the docs. This tells the user "this field is going away soon" which is clearly different than "this is a new field that hasn't yet been documented". To avoid all confusion, the field is documented and gets removed from the docs when it gets removed from the implementation. We haven't used the deprecation feature in the docs yet. Once this change goes in and I update the reference docs on istio.io, we can look at the rendering and decide whether we want something different there. If I recall correctly, deprecated fields just get rendered in a different color and it says something about being deprecated if you hover over the field. We could also use striketrhough or other approaches... |
|
New changes are detected. LGTM label has been removed. |
|
@ozevren: The following test failed, say
DetailsInstructions 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. |
rshriram
left a comment
rshriram
left a comment
There was a problem hiding this comment.
Everything that is marked as deprecated and unused in this PR - we should not even be displaying them. These were removed (implementation) by 1.0 but not from the mesh config. So you can define them but it will have no effect at all.
I am also pretty certain that we did not use any of these fields in the 1.0 release (helm). No one could have used them either as they would have had no effect.
|
closing as another PR that does the same has merged |
5 participants
Based on diff of master against 76349c5 in release-1.0 branch.