-
Notifications
You must be signed in to change notification settings - Fork 12
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
toJson() / fromJson() not symmetric in respect to strain order #71
Comments
In the following example, the config sent:
config received:
|
the problem is, that even YAML doesn't mandate an order for mappings: https://yaml.org/spec/1.2/spec.html#id2764044
|
I see the following solutions:
|
my preference: 4 > 2 > 3> 1 @trieloff WDYT? |
With #20 (conditions language) we might have a good shot at option 4 (which I think is preferred by @kptdobe): sort the strains from most specific condition to least specific condition. CSS has some kind of specificity rules (https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity) and I think we could construct a partially ordered set on our conditions language, where we apply a set of rules to determine if two conditions are more or less specific, or not comparable or equal. Rules:Property Matches between different properties are never comparableExample:
For
|
I turned it into a separate issue (#72) @tripodsan I'd add a |
I think we should stick to KISS and use a YAML sequence (list) for the strains. the evaluation order is the very simple to explain:
For backward compatibility to those thousands of existing configs, the YAML import will support both, sequence and map. but the YAML and JSON export, will always write the strains as a list. |
btw, the yaml parser we use has an option to use javascript maps for maps. I believe this would keep the order stable. |
If I think of examples out there where YAML order matters, travis and circleci configs come to mind. In those configs, the run order of commands matters a lot - and they both use sequences to solve this problem. The sequence solution, to me, follows the principle of least astonishment. |
# [0.9.0](v0.8.4...v0.9.0) (2019-03-27) ### Features * **configuration:** Strains can be and should be specified as an ordered list ([38b5e9d](38b5e9d)), closes [#71](#71)
🎉 This issue has been resolved in version 0.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
In the current state, the "map" strains config rewrite generates a config that looks like this: https://github.com/adobe/project-helix.io/blob/test-map-strains/helix-config.yaml And
|
In the current state, the "sequence" strains seems to be handled properly during deploy (see https://github.com/adobe/project-helix.io/blob/test-sequence-strains/helix-config.yaml). But publish fails!
|
In both cases, we cannot publish. The error seems to be the same. Some schema error. |
I assume that if we write some tests for the schemas, we'll find the error quickly. |
I think the error is that |
For the 1st, it seems to happen before hitting |
We do not have tests on the "map" after the yaml rewrite, I am sure we can easily reproduce the error. |
Once this workflow completes, you can try the "sequence" publish again: https://circleci.com/workflow-run/2e744869-fb7e-403a-bee0-ae6a9b75ecf2 |
You might be right, I cannot reproduce in helix-shared with both config rewritten. |
Sorry, no, I can reproduce with the "map" rewritten config. |
i would ignore/delete all map tests. they are not relevant anymore and i would deprecate the format anyways. |
What are we doing about adobe/helix-cli#695 |
i don’t understand. the map type is still read but saved as sequence style. the json is exported sequence style. and simulator and helix-publish should have the new shared. |
If we ignore the map tests, we ignore backward compatibility. It could be fine (all configs must be updated) but then we must also get rid of the code handling it. |
We need to upgrade the tests. This is the proof on non-backward compatibility ;) |
I just updated the tests here: adobe/helix-simulator#172 |
Ah... if you look here: https://github.com/adobe/project-helix.io/blob/test-map-strains/helix-config.yaml |
I think the strains.schema is wrong. What do |
ah ok, found the definition at the beginning of the file ;) |
adobe/helix-cli@ae2769d is the fix (CI pending) for adobe/helix-cli#695 |
OK... Found it!!! The The error message is huge because of the embedded Now, next question is how to solve that:
I vote for 1. |
Well... especially that the |
See #80 |
I created: to follow up on this. |
Description
the order of properties is not guaranteed to be stable in objects, nor in the JSON serialization. this causes the order of strains to be undeterministic. Since the condition matching relies on the order, it is important to keep it stable.
To Reproduce
not so easy, since the object hash is mostly stable.
Version:
The text was updated successfully, but these errors were encountered: