-
Notifications
You must be signed in to change notification settings - Fork 99
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
Allow numbers again #414
Allow numbers again #414
Conversation
Revert the changes in cnabio#257 which updated the spec to disallow non-integer values. We misunderstood the canonical json spec and were using a library that didn't support numbers. I have an open PRs for cnab-go which tested out switching to a library that support numbers in canonical json and it works great. * cnabio/cnab-go#247 * cnabio/cnab-go#248 So now fields such as default, maximum, minimum, etc can use numbers again! Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@technosophos @chris-crone I believe this would be a patch to the spec version? I think we will want to bump the spec version after this is merged. |
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.
Oooh, nice to be able to get rid of the custom definitions schema 🎉
Only comment is around continuing support for integer
in addition to number
.
101-bundle-json.md
Outdated
@@ -415,7 +415,7 @@ of how to use `definitions` along with `parameters` and `outputs` can be seen in | |||
- `required`: Parameter validation requiring the properties named in the user-provided object include the specified list of properties. MUST be an array of strings. (OPTIONAL) | |||
- `then`: Parameter validation requiring that the user-provided value match the specified schema. Only matches if the user-provided value matches the schema provided in the `if` property. MUST be a JSON schema. (OPTIONAL) | |||
- `title`: Short, human-readable descriptive name for the field. Can be used to decorate a user interface. MUST be a string. (OPTIONAL) | |||
- `type`: Parameter validation requiring that the user-provided value is either a "null", "boolean", "object", "array", "string", or "integer". MUST be a string or an array of strings with unique elements. If you need to represent another numeric type, upscale to an integer or use a string type and convert within your bundle. (OPTIONAL) | |||
- `type`: Parameter validation requiring that the user-provided value is either a "null", "boolean", "object", "array", "string", or "number". MUST be a string or an array of strings with unique elements. (OPTIONAL) |
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.
We can keep "integer" as well, right? I think it's a valid JSON Schema construct distinct from "number".
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
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, with the mention that the canonical JSON library used in Signy will have to be updated as well.
I'll submit a PR to signy as I already went through the switch for cnab-go. Here are the follow-up PRs to this spec change for cnab-go: |
Per CNAB Spec cnabio/cnab-spec#414, we want to support numbers in our canonical json representation. The library we are currently using, github.com/docker/go/json, does not support this. So I am migrating us to the library mentioned in the spec as being compliant. This is the same library used by cnab-go. I was not able to completely remove the import of the old library because TUF uses its RawMessage struct, which is a very simple wrapper around a byte array. If we are intersted we can try to get TUF to use an interface instead of a hard-coded struct type so that we can drop the dependency on the other canonical json library. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
This doesn't break the existing interpretation so I agree! |
Per CNAB Spec cnabio/cnab-spec#414, we want to support numbers in our canonical json representation. The library we are currently using, github.com/docker/go/json, does not support this. So I am migrating us to the library mentioned in the spec as being compliant. This is the same library used by cnab-go. I was not able to completely remove the import of the old library because TUF uses its RawMessage struct, which is a very simple wrapper around a byte array. If we are intersted we can try to get TUF to use an interface instead of a hard-coded struct type so that we can drop the dependency on the other canonical json library. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
* go mod tidy Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Switch canonical json libraries Per CNAB Spec cnabio/cnab-spec#414, we want to support numbers in our canonical json representation. The library we are currently using, github.com/docker/go/json, does not support this. So I am migrating us to the library mentioned in the spec as being compliant. This is the same library used by cnab-go. I was not able to completely remove the import of the old library because TUF uses its RawMessage struct, which is a very simple wrapper around a byte array. If we are intersted we can try to get TUF to use an interface instead of a hard-coded struct type so that we can drop the dependency on the other canonical json library. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Make linter happy Rename package to not use an underscore Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Remove MarshalToRawMessage This wasn't necessary like I originally thought since []byte converts cleanly to RawMessage by the compiler with any extra code. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Revert the changes in #257 which updated the spec to disallow non-integer values. We misunderstood the canonical json spec and were using a library that didn't support numbers.
I have an open PRs for cnab-go which tested out switching to a library that support numbers in canonical json and it works great.
So now fields such as default, maximum, minimum, etc can use numbers again! 🎉
Closes #413