Schema users#1379
Conversation
49774e8 to
4b30298
Compare
- Support types as list for doc rendering versus single {"type": "string"}
- Add _sort_property_order so 'array' and 'object' types are sorted last
when multiple types are listed via oneOf or type lists
- Decorate enum values with double back-ticks and replace "string" type
- Add _flatten_schema_refs support for extracting $defs into schema for
references nested under "items" and "oneOf" keys
- Update unit tests for markdown doc coverage
4b30298 to
f84c7b2
Compare
TheRealFalcon
left a comment
There was a problem hiding this comment.
Some really nice changes here. Thanks!
Unfortunately, there's some cruft in this module that we need to deal with. A user key is valid as a means to replace the default user. Also, the users, groups, individual user, and individual group definitions can all technically be a list, dict, or string.
E.g., the following is (somehow) valid:
#cloud-config
user: foo
users: foobarAlso, user definition is missing the following keys:
- plain_text_passwd, hashed_passwd, create_groups
Unrelated, we should probably slip in removing line 636 of the schema.
"partition": {
"type": ["string", "integer"],
"oneOf": [
{
"type": "string",
"enum": ["auto", "any", "none"]
},
{"type": "integer"}
],type is redundant here and now renders the docs as partition: (string/integer/string/integer) .
Everything else looks really good though!
| # Password-based login is rejected, but the github user TheRealFalcon | ||
| # and the launchpad user falcojr can SSH as newsuper. The default | ||
| # shell for newsuper is bash instead of system default. | ||
| users: |
There was a problem hiding this comment.
I think it'd be worthwhile to add the default user to the example and make it the first example. Something like this is probably the most common use case, so I think it makes sense to have it be first. We could remove default from example 3.
| "groups": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/$defs/users_groups.group_with_users" |
There was a problem hiding this comment.
Not a problem but just wondering Is there a reason you went with refs here rather than keeping the definition inline? Do you think it's worth moving the refs to be directly above this definition so they're easier to find?
There was a problem hiding this comment.
I think this makes sense to avoid the reference, I had originally thought that items might be more complex than it ended up being. I think we should reserve $defs for things that are duplicated, or where the size of the schema is so large in oneOfs that it makes it hard to see the alternative schemas defined within a certain scope. So I'll drop /users_groups.group_with_users and make it inline, but I'd lean toward keeping users_groups.user so we can easily see that funky "default" vs <users_groups.user> schema
Strange works for me $ PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --annotate -c ug.yaml
#cloud-config
user: foo
users: foobar # E1
# Errors: -------------
# E1: 'foobar' is not of type 'array' |
|
Ooooh, sorry, I meant valid as in it's valid to the code, so it needs to be valid to the schema too. |
Added a DEPRECATED warning as we should be able to replace this fully with
Added to schema
agreed/done |
- Add deprecated 'user' schema support pointing at 'users'. - groups key can support comma-separated strings of groups instead of list of dicts - users can be string of comma-separated usernames instead of list
I don't want to add schema validation yet as is makes duplicated markdown confusing at the moment.
When hidden is True, hide all docs for that object. When hidden is a list hide only docs for matching top-level keys. Fixes: SC-897
568f5e2 to
84951b4
Compare
|
Top level users is still defined as array, but should support string, object, array |
|
Adding some more context to my last comment, I think users: "aaa,bbb"users:
aaa:
shell: /bin/bash
bbb: trueusers:
- ccc
- name: ddd
shell: /bin/bash
- [aaa, bbb] |
Fixed those missing cases here too and validated each schema suggested with unittests and |
TheRealFalcon
left a comment
There was a problem hiding this comment.
This looks good to me now, and the unit tests (including lowest support) pass for me now. If you didn't have anything else left here, feel free to merge!
Proposed Commit Message
Additional Context
Test Steps
Checklist: