treefile.rs: Deny unknown fields by default #1459
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Let's not make the same mistake we did with JSON where typoing a field means it's silently ignored. This actually caught a bug in a YAML usage we had: ``` error: Failed to load YAML treefile: unknown field `install_langs`, expected one of ... `install-langs` ... ``` Yes, this is a compatibility break with the feature we just announced but...I seriously doubt anyone (that isn't known to me) has converted yet, and if they are excited enough to start using a two-week-old feature they can adjust.
cgwalters
added a commit
to cgwalters/os
that referenced
this pull request
Jul 13, 2018
I was going to do some treefile changes, ended up doing coreos/rpm-ostree#1459 which turned up the bug in this repo. Let's actually fix it by just installing all languages. I'd actually like to do man pages too, but that will come separately after discussion.
cgwalters
added a commit
to cgwalters/os
that referenced
this pull request
Jul 13, 2018
I was going to do some treefile changes, ended up doing coreos/rpm-ostree#1459 which turned up the bug in this repo. Let's actually fix it by just installing all languages. I'd actually like to do man pages too, but that will come separately after discussion.
cgwalters
added a commit
to cgwalters/os
that referenced
this pull request
Jul 16, 2018
I was going to do some treefile changes, ended up doing coreos/rpm-ostree#1459 which turned up the bug in this repo.
|
Hmm, this also means that treefiles that use newer keys will no longer work with older rpm-ostree versions... which I think makes sense, so 👍. |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jul 17, 2018
Let's not make the same mistake we did with JSON where typoing a field means it's silently ignored. This actually caught a bug in a YAML usage we had: ``` error: Failed to load YAML treefile: unknown field `install_langs`, expected one of ... `install-langs` ... ``` Yes, this is a compatibility break with the feature we just announced but...I seriously doubt anyone (that isn't known to me) has converted yet, and if they are excited enough to start using a two-week-old feature they can adjust. Closes: #1459 Approved by: jlebon
|
💔 Test failed - status-atomicjenkins |
|
@rh-atomic-bot retry |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jul 17, 2018
Let's not make the same mistake we did with JSON where typoing a field means it's silently ignored. This actually caught a bug in a YAML usage we had: ``` error: Failed to load YAML treefile: unknown field `install_langs`, expected one of ... `install-langs` ... ``` Yes, this is a compatibility break with the feature we just announced but...I seriously doubt anyone (that isn't known to me) has converted yet, and if they are excited enough to start using a two-week-old feature they can adjust. Closes: #1459 Approved by: jlebon
|
💔 Test failed - status-atomicjenkins |
|
Looks like an rpmmd race. |
Ah yes...this is the issue I was trying to fix. |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jul 17, 2018
Let's not make the same mistake we did with JSON where typoing a field means it's silently ignored. This actually caught a bug in a YAML usage we had: ``` error: Failed to load YAML treefile: unknown field `install_langs`, expected one of ... `install-langs` ... ``` Yes, this is a compatibility break with the feature we just announced but...I seriously doubt anyone (that isn't known to me) has converted yet, and if they are excited enough to start using a two-week-old feature they can adjust. Closes: #1459 Approved by: jlebon
|
💔 Test failed - status-atomicjenkins |
|
OK so for now I removed the arch bits, as we weren't actually testing them in the compose tests in either case (JSON/YAML). I'll do a separate PR for parsing the arch-specific packages in the YAML path? |
|
Yeah, SGTM! |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jul 17, 2018
Let's not make the same mistake we did with JSON where typoing a field means it's silently ignored. This actually caught a bug in a YAML usage we had: ``` error: Failed to load YAML treefile: unknown field `install_langs`, expected one of ... `install-langs` ... ``` Yes, this is a compatibility break with the feature we just announced but...I seriously doubt anyone (that isn't known to me) has converted yet, and if they are excited enough to start using a two-week-old feature they can adjust. Closes: #1459 Approved by: jlebon
|
💔 Test failed - status-atomicjenkins |
Hmm. |
jlebon
reviewed
Jul 18, 2018
| "packages-ppc64le": ["grub2", "ostree-grub2"], | ||
|
|
||
| "packages-x86_64": ["grub2", "grub2-efi", "ostree-grub2", | ||
| "efibootmgr", "shim"], |
There was a problem hiding this comment.
Ahh, we need to merge the x86_64 list back into packages for now probably.
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jul 18, 2018
Let's not make the same mistake we did with JSON where typoing a field means it's silently ignored. This actually caught a bug in a YAML usage we had: ``` error: Failed to load YAML treefile: unknown field `install_langs`, expected one of ... `install-langs` ... ``` Yes, this is a compatibility break with the feature we just announced but...I seriously doubt anyone (that isn't known to me) has converted yet, and if they are excited enough to start using a two-week-old feature they can adjust. Closes: #1459 Approved by: jlebon
|
💔 Test failed - status-atomicjenkins |
jlebon
force-pushed
the
yaml-treefile-arch-packages
branch
from
40e3d8b to
654e849
Compare
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jul 19, 2018
Let's not make the same mistake we did with JSON where typoing a field means it's silently ignored. This actually caught a bug in a YAML usage we had: ``` error: Failed to load YAML treefile: unknown field `install_langs`, expected one of ... `install-langs` ... ``` Yes, this is a compatibility break with the feature we just announced but...I seriously doubt anyone (that isn't known to me) has converted yet, and if they are excited enough to start using a two-week-old feature they can adjust. Closes: #1459 Approved by: jlebon
|
💔 Test failed - status-atomicjenkins |
|
Hmm, |
This should help out with determining what steps take the most time.
|
bot, retest this please |
jlebon
force-pushed
the
yaml-treefile-arch-packages
branch
from
bf8d9ee to
a5cb2e1
Compare
|
Hmm, OK so it's not so much that the test composes themselves are taking longer, but the prep leading up to them went from ~9m to ~20m. Though in #1406, it just went fine. So it's definitely not consistent. |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jul 21, 2018
This should help out with determining what steps take the most time. Closes: #1459 Approved by: cgwalters
|
☀️ Test successful - status-atomicjenkins |
cgwalters
added a commit
to cgwalters/rpm-ostree
that referenced
this pull request
Jul 23, 2018
Follow up to: coreos#1459 We now honor arch-specific pacakges in YAML, and reject unknown architectures. I looked a little bit at how to avoid having hardcoded arch lists, but it doesn't seem worth it right now.
cgwalters
added a commit
to cgwalters/rpm-ostree
that referenced
this pull request
Jul 23, 2018
Follow up to: coreos#1459 We now honor arch-specific packages in YAML, and reject unknown architectures. I looked a little bit at how to avoid having hardcoded arch lists, but it doesn't seem worth it right now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Let's not make the same mistake we did with JSON where typoing a
field means it's silently ignored. This actually caught a bug
in a YAML usage we had:
Yes, this is a compatibility break with the feature we just announced
but...I seriously doubt anyone (that isn't known to me) has converted
yet, and if they are excited enough to start using a two-week-old feature
they can adjust.