Skip to content

Conversation

zx80
Copy link

@zx80 zx80 commented Jul 2, 2025

Fix to obvious schema issues. There are others less straightforward to fix, though.

Fix to obvious schema issues. There are others less straightforward to fix, though.
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

Sorry but I do not see these errors as obvious. If you can add at least one positive/negative tests, it would much easier to understand them and also to prevent further changes that might break the schema.

@ssbarnea ssbarnea marked this pull request as draft July 4, 2025 15:16
@zx80
Copy link
Author

zx80 commented Jul 5, 2025

Sorry but I do not see these errors as obvious.

The current schema itself is obviously wrong because what is written does not make sense from a semantical point of view. However, how to fix it depends on the underlying intention which failed to materialize in the schema, and is really just guessing from my part because I do not know the real/detailed expectations.

For these two fixes on the meta json file, which seems to correspond more or less to this description and this other:

  1. The else does not have a corresponding if thus is dead code, which I consider an obvious error. My guess is that it is badly nested and belongs to the if/then which is just above, but maybe it should just be removed.

  2. Having additionalProperties inside properties is a very classic schema error, unless you are really writing a JSON Schema meta schema, in which case the associated value would not be false. My guess is that the element was expected at the upper level where it actually means something, but maybe the entry just needs to be removed, I cannot decide simply.

What is really expected can probably be infered from the source code of the ansible commands which uses the described data structure.

The 2 other issues I reported in #4660 have a strange schema semantics which demonstrates that writing schemas is error prone, but I could not make an easy guess about what the schema writer had in mind.

If you can add at least one positive/negative tests, it would much easier to understand them and also to prevent further changes that might break the schema.

I am sorry, I was just planing to point out bugs in a schema, not to do debugging in an unfamiliar environment.

- Attempt at having a sane semantics on `GalaxyInfo` conditionals.
- Simply remove strange `additionalProperties` inside a `properties`.
@zx80
Copy link
Author

zx80 commented Jul 5, 2025

I've updated the GalaxyInfo structure to have a conditional with a clearer semantics and remove the strangeness.

Tox does not like it because it seems to actually check the strange then semantics which was to forbid all properties to appear together, whereas any subset was still okay (which is probably the non intentional part from the schema spec).

❓ what is the actual expectation when standalone is false? Is it to actually only forbid all props together, or to forbid any of these props to appear?

if not standalone, we interpret the previous schema as "these props are forbidden", which is not the same as "these props are forbidden when they are all together".
@ssbarnea
Copy link
Member

ssbarnea commented Sep 4, 2025

@zx80 you can update the test fixtures for the schema to allow it to pass. They are there in order to ensure that whatever changes we make to the schema, the resulting error from the two supported validators do not get worse. You kind of change is exactly why we added the tests because it can greatly improve some error messages or the opposite, just due to the way the validators were implemented.

Even if you change might be "better", if the error messages are worse we will stick with less optimal schema as the most important aspect is to keep the validation failures as clear as we can.

@ssbarnea
Copy link
Member

ssbarnea commented Sep 9, 2025

Closing due to lack of updates but fee free to reopen if you restart working on it.

@ssbarnea ssbarnea closed this Sep 9, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in 🧰 devtools project board Sep 9, 2025
@zx80
Copy link
Author

zx80 commented Sep 11, 2025

Well, my intention was really to report the issue, not to fix it…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants