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

Strengthen the schema w/r/t format, required fields #397

Merged
merged 70 commits into from
Jul 28, 2022

Conversation

johnrandolph
Copy link
Collaborator

No description provided.

@johnrandolph johnrandolph requested a review from grafnu July 26, 2022 20:45
@johnrandolph johnrandolph marked this pull request as ready for review July 27, 2022 16:53
@@ -15,10 +15,14 @@ if [[ -z $origin ]]; then
origin=faucetsdn
fi

git fetch $origin --tags 2>/dev/null
git fetch $origin --tags # 2>/dev/null
Copy link
Collaborator

Choose a reason for hiding this comment

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

unexpected #

},
"sha256": {
"type": "string"
"type": "string",
"pattern": "^[0-9A-Za-z]{64}$"
Copy link
Collaborator

Choose a reason for hiding this comment

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

what do you think about just making it all-lower or all-upper? Reason being, then it's easier to do a naive "string compare" to see if "hash matched" -- otherwise it takes extra code that somebody is likely to get wrong... I'd rather force the generation to be all-lower than rely on the equality check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm fine with all lowercase because that's customary. I was wondering if device makers would have other opinions, especially from non-Unix platforms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also this is a mistake and it should be a-f not a-z.

}
},
"oneOf": [
{ "required": ["content_type", "base64"] },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have expected oneOf { base64, url } (transport mechanism), and then sha256 is always required, and then content_type is optional? I suppose sha256 isn't required with base64... But doesn't content_type potentially apply to all transport mechanisms? Oh wait, is it because content_type is included in the HTTP response?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, content_type comes from http so we don't want to allow overriding it--feels dangerous to me

@johnrandolph johnrandolph merged commit acc8dc7 into faucetsdn:master Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants