-
Notifications
You must be signed in to change notification settings - Fork 919
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
Overriding values inside JSON and YAML #5449
Conversation
Welcome @sophiefeifeifeiya! It looks like this is your first PR to karmada-io/karmada 🎉 |
a88c656
to
bcefab3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5449 +/- ##
=======================================
Coverage 34.84% 34.85%
=======================================
Files 645 645
Lines 44861 44861
=======================================
+ Hits 15633 15636 +3
+ Misses 28021 28020 -1
+ Partials 1207 1205 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bc905de
to
4ef6cf9
Compare
docs/proposals/override-values-inside-structured-data-files/README.md
Outdated
Show resolved
Hide resolved
docs/proposals/override-values-inside-structured-data-files/README.md
Outdated
Show resolved
Hide resolved
docs/proposals/override-values-inside-structured-data-files/README.md
Outdated
Show resolved
Hide resolved
docs/proposals/override-values-inside-structured-data-files/README.md
Outdated
Show resolved
Hide resolved
docs/proposals/override-values-inside-structured-data-files/README.md
Outdated
Show resolved
Hide resolved
docs/proposals/override-values-inside-structured-data-files/README.md
Outdated
Show resolved
Hide resolved
docs/proposals/override-values-inside-structured-data-files/README.md
Outdated
Show resolved
Hide resolved
4ef6cf9
to
9240cb1
Compare
642d30a
to
9fd505d
Compare
9fd505d
to
86ea873
Compare
UpdatesSome updates after testing:
In this process, the explicit type tags are removed, and the values are transformed into the data types indicated by the tags. However, this previously identified issue has now been resolved.
The proposal is modified again, and you can refer to new modified proposal. ConclusionThus, for the YAML implementation, Plan 1 is preferable. However, it’s important to inform users that: What do you guys think about? |
At the same time cc @RainbowMango |
docs/proposals/override-values-inside-configurations-of-different-data-formats/README.md
Outdated
Show resolved
Hide resolved
docs/proposals/override-values-inside-configurations-of-different-data-formats/README.md
Outdated
Show resolved
Hide resolved
9db55c7
to
e56ec27
Compare
4db13d4
to
624c63b
Compare
624c63b
to
529c3ed
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.
Leaving some nits, otherwise LGTM.
@chaunceyjiang @Patrick0308 Please take another look, if no further comments I guess we can move it forward then.
529c3ed
to
20fac46
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.
@chaunceyjiang: GitHub didn't allow me to request PR reviews from the following users: Patrick0308. Note that only karmada-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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-sigs/prow repository. |
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.
Some nit hits. LGTM
Signed-off-by: sophie <[email protected]>
d99e513
to
0682184
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
@chaunceyjiang @Patrick0308 Do you have any further comments?
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chaunceyjiang 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 design
/kind documentation
What this PR does / why we need it:
Overriding values inside JSON and YAML to simplify modifications.
Which issue(s) this PR fixes:
Fixes #5330
Special notes for your reviewer:
Related PR: #5329
Does this PR introduce a user-facing change?: