Skip to content
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

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented Oct 7, 2024

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

@Joibel Joibel marked this pull request as ready for review October 7, 2024 11:02
@Joibel Joibel changed the title Json templating followup docs: json templating followup for upgrading notes Oct 7, 2024
@Joibel Joibel added the area/docs Incorrect, missing, or mistakes in docs label Oct 7, 2024
@Joibel Joibel added this to the v3.6.0 milestone Oct 7, 2024
@@ -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.
Copy link
Member

@agilgur5 agilgur5 Oct 7, 2024

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:

Suggested change
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)

Copy link
Member Author

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

### 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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@agilgur5 agilgur5 changed the title docs: json templating followup for upgrading notes docs: json templating upgrading notes Oct 7, 2024
@agilgur5 agilgur5 self-assigned this Oct 7, 2024
@agilgur5 agilgur5 added the area/templating Templating with `{{...}}` label Oct 7, 2024
@agilgur5 agilgur5 changed the title docs: json templating upgrading notes docs: add 3.6 json templating upgrading notes Oct 7, 2024
@@ -5,6 +5,11 @@ the [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/#summar

## Upgrading to v3.6

### JSON templating fix
Copy link
Member

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)

Copy link
Member Author

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.

Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

LGTM

@Joibel Joibel merged commit f9aa2b1 into argoproj:main Oct 17, 2024
16 checks passed
@Joibel Joibel deleted the json-templating-followup branch October 17, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs area/templating Templating with `{{...}}`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.6.0: JSON templating change release notes & test cases
4 participants