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

Provide error on duplicate YAML keys in strict mode? #881

Open
GUI opened this issue Nov 13, 2023 · 3 comments
Open

Provide error on duplicate YAML keys in strict mode? #881

GUI opened this issue Nov 13, 2023 · 3 comments
Assignees
Labels
enhancement This issue is a feature request priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome.

Comments

@GUI
Copy link

GUI commented Nov 13, 2023

Describe the problem/challenge you have

It's easy to introduce bugs into YAML due to duplicative keys. For example:

key: value1
key: value2

Will evaluate to:

key: value2

I realize YAML allows duplicate keys and this evaluation makes sense, but in large YAML documents, it can be easy to overlook duplicate keys that are more spread out in the document, which can lead to bugs (since it may not be immediately obvious that some value will get overwritten later on in the document).

Describe the solution you'd like

I was wondering if YTT could perhaps help guard against this type of situation, since it seems like duplicative keys are usually a mistake within a single YAML document. In particular, I was wondering if this might be a good fit for ytt's "strict" mode, since its stated goal is: "tries to remove any kind of ambiguity in user’s intent when parsing YAML." Duplicative keys seem like they fit this bill, so I was wondering if you all think this would be a useful addition to strict mode.

Currently strict mode doesn't care about duplicate keys, and just uses the last value:

printf "key: value1\nkey: value2" | ytt -f- --strict
key: value2

But I might envision strict mode behaving something like this:

printf "key: value1\nkey: value2" | ytt -f- --strict
ytt: Error: Unmarshaling YAML template 'stdin.yml': yaml:
  Strict parsing:
    Found duplicate key 'key'

But non-strict mode could continue to work as-is.


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@GUI GUI added carvel triage This issue has not yet been triaged for relevance enhancement This issue is a feature request labels Nov 13, 2023
@prembhaskal prembhaskal self-assigned this Nov 17, 2023
@prembhaskal
Copy link
Member

prembhaskal commented Nov 17, 2023

@GUI Have you considered using a linting tool like yamllint?

I will check if this is something viable to include in lib, and how unmarshalling currently handles duplicate because go yaml lib seems to returns error as per this.

Copy link

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

@github-actions github-actions bot added the stale This issue has had no activity for a while and will be closed soon label Dec 28, 2023
@github-actions github-actions bot closed this as completed Jan 2, 2024
@renuy renuy reopened this Jan 2, 2024
@renuy renuy removed the stale This issue has had no activity for a while and will be closed soon label Jan 2, 2024
@prembhaskal
Copy link
Member

So i finally got around to check this.
Ytt is removing the duplicate keys and retaining only the last one here.
Plain yaml.v3 unmarshal rejects duplicate keys (https://go.dev/play/p/WEOlmjSJ0UQ)

So I think your suggestion is valid and it seems feasible that we can check in strict mode for unique keys.
I will move the issue to backlog for now.

@prembhaskal prembhaskal added priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome. and removed carvel triage This issue has not yet been triaged for relevance priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. labels Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a feature request priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome.
Projects
Status: Prioritized Backlog
Development

No branches or pull requests

3 participants