-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
docs: add 3.6 json templating upgrading notes #13720
Conversation
095e148
to
ea6a49c
Compare
docs/upgrading.md
Outdated
@@ -5,6 +5,11 @@ the [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/#summar | |||
|
|||
## Upgrading to v3.6 | |||
|
|||
### JSON templating fix | |||
|
|||
When accessing a structure or array from json structure in a `jsonpath` expression you could end up getting a golang representation of the structure in error. |
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.
While the test cases in #12909 are jsonpath
specific, the code is not, as far as I understand, otherwise it could have been applied to the jsonpath
helper specifically.
The wording is also duplicative and inconsistent:
When accessing a structure or array from json structure in a `jsonpath` expression you could end up getting a golang representation of the structure in error. | |
When accessing a map or array from JSON in a `jsonpath` expression, you would get a Golang representation. |
- consistently use "JSON", not both "json" and "JSON" (esp in the same section)
- "Golang" should similarly be capitalized as a proper noun
- simplify, per k8s style guide
- "structure" is not needed (was previously used 3 times in one sentence) and is ambiguous, whereas "map" is a data structure consistent with "array"
- "in error" is confusing/ambiguous as there is no error message. It's not needed either, we can just say what was returned before and what it returns now. (also whether that was concretely a mistake before is not clear either)
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.
Done, and slightly simplified again
docs/upgrading.md
Outdated
### JSON templating fix | ||
|
||
When accessing a structure or array from json structure in a `jsonpath` expression you could end up getting a golang representation of the structure in error. | ||
This will now return the json representation as hoped - see #12909. |
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.
This will now return the json representation as hoped - see #12909. | |
This now returns plain JSON. |
- simplify, per k8s style guide
I'm also not sure we need to reference the PR and I'm not sure that referencing it would be helpful. They sometimes are and aren't referenced in the upgrading notes, but when they are it is part of the section heading and not in the contents. A header reference makes more sense to me as a way to correlate to the changelog items.
Also the PR number is not automatically linked
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.
Done
docs/upgrading.md
Outdated
@@ -5,6 +5,11 @@ the [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/#summar | |||
|
|||
## Upgrading to v3.6 | |||
|
|||
### JSON templating fix |
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.
I'd probably move this near the legacy patch
pod
removal if the thought is that it shouldn't have much impact. I.e. order of importance (then all the metrics changes as things after it could be missed accidentally since it's a large section)
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.
I've moved it to the bottom as least important.
Signed-off-by: Alan Clucas <alan@clucas.org>
Signed-off-by: Alan Clucas <alan@clucas.org>
ea6a49c
to
7fa9d77
Compare
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
Fixes #13541
Follow up to add upgrading notes to #12909
Motivation
#12909 is a breaking change and wasn't listed in the upgrading notes
Modifications
Add upgrading notes for this change. The change is subtle and probably affects few users. Some may have worked around it as per #8930 and can optionally remove
toJson
from expressions, but this is not necessary.Verification
make docs
passes