schema: add JSON defs for modules resize-salt (SC-654)#1341
Merged
Conversation
Includes: - cc_resizefs - cc_resolv_conf - cc_rh_subscription - cc_rightscale_userdata - cc_rsyslog - cc_runcmd - cc_salt_minion
TheRealFalcon
changed the title
Contributor
Author
|
I've seen the flake8 errors, but I figure there'll be other comments to address, so I'll wait on those. |
blackboxsw
requested changes
Mar 22, 2022
Collaborator
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Not for this PR, but I really don't like that rsyslog_config silently sets default values if invalid values are provided in config. It's probably worth us taking a separate bug about this for future cleanup.
All else looks pretty good for this PR so take or leave comments as you want with the exception of my question on manage_resolv_conf default value. Maybe I'm reading the code wrong there but it doesn't look like we default to True. I'll review your responses tomorrow on the rest of the items and we can close out on this branch. Thanks again
| are configured correctly. | ||
|
|
||
| When using a config drive and a RHEL-like system, resolv.conf will also be | ||
| managed automatically due to the available information provided for dns |
Collaborator
There was a problem hiding this comment.
Suggested change
| managed automatically due to the available information provided for dns | |
| managed automatically due to the available information provided for DNS |
Collaborator
There was a problem hiding this comment.
probably want to replace dns/DNS throughout text.
| will use sysconfig, this module is likely to be of little use unless those | ||
| are configured correctly. | ||
|
|
||
| When using a config drive and a RHEL-like system, resolv.conf will also be |
Collaborator
There was a problem hiding this comment.
Suggested change
| When using a config drive and a RHEL-like system, resolv.conf will also be | |
| When using a :ref:`datasource_config_drive` and a RHEL-like system, resolv.conf will also be |
|
|
||
| When using a config drive and a RHEL-like system, resolv.conf will also be | ||
| managed automatically due to the available information provided for dns | ||
| servers in the config drive network format. For those that with to have |
Collaborator
There was a problem hiding this comment.
Suggested change
| servers in the config drive network format. For those that with to have | |
| servers in the :ref:`network_config_v2` format. For those that with to have |
Comment on lines
34
to
36
| **Config keys**:: | ||
|
|
||
| CLOUD_INIT_REMOTE_HOOK=<url> |
Collaborator
There was a problem hiding this comment.
I'd change this to
Suggested change
| **Config keys**:: | |
| CLOUD_INIT_REMOTE_HOOK=<url> | |
| **Raw user data schema**:: | |
| CLOUD_INIT_REMOTE_HOOK=<url> |
Comment on lines
+40
to
+41
| maas: "192.168.1.1" | ||
| juju: "10.0.4.1" |
Collaborator
There was a problem hiding this comment.
Let's ditch unnecessary quotes where we can to aid in readability, simplicity of config values
Suggested change
| maas: "192.168.1.1" | |
| juju: "10.0.4.1" | |
| maas: 192.168.1.1 | |
| juju: 10.0.4.1 |
| "properties": { | ||
| "username": { | ||
| "type": "string", | ||
| "description": "The username to use. Must be used with password. Should not be used with activation-key or org" |
Collaborator
There was a problem hiding this comment.
Markdown sugar for key names
Suggested change
| "description": "The username to use. Must be used with password. Should not be used with activation-key or org" | |
| "description": "The username to use. Must be used with password. Should not be used with ``activation-key`` or ``org``" |
| }, | ||
| "password": { | ||
| "type": "string", | ||
| "description": "The password to use. Must be used with username. Should not be used with activation-key or org" |
Collaborator
There was a problem hiding this comment.
Suggested change
| "description": "The password to use. Must be used with username. Should not be used with activation-key or org" | |
| "description": "The password to use. Must be used with username. Should not be used with ``activation-key`` or ``org``" |
| }, | ||
| "activation-key": { | ||
| "type": "string", | ||
| "description": "The activation key to use. Must be used with org. Should not be used with username or password" |
Collaborator
There was a problem hiding this comment.
Suggested change
| "description": "The activation key to use. Must be used with org. Should not be used with username or password" | |
| "description": "The activation key to use. Must be used with org. Should not be used with ``username`` or ``password``" |
| }, | ||
| "org": { | ||
| "type": "integer", | ||
| "description": "The organization number to use. Must be used with activation-key. Should not be used with username or password" |
Collaborator
There was a problem hiding this comment.
Suggested change
| "description": "The organization number to use. Must be used with activation-key. Should not be used with username or password" | |
| "description": "The organization number to use. Must be used with ``activation-key``. Should not be used with ``username`` or ``password``" |
| }, | ||
| "service-level": { | ||
| "type": "string", | ||
| "description": "The service level to use when subscribing to RH repositories. auto-attach must be true for this to be used" |
Collaborator
There was a problem hiding this comment.
Suggested change
| "description": "The service level to use when subscribing to RH repositories. auto-attach must be true for this to be used" | |
| "description": "The service level to use when subscribing to RH repositories. ``auto-attach`` must be true for this to be used" |
blackboxsw
approved these changes
Mar 23, 2022
Collaborator
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Thanks for the quick fixes. +1
This was referenced May 12, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Commit Message
Additional Context
There's a legacy syntax for rsyslog that became legacy 6 years ago. It will still work but I removed all references to it, don't include it in the schema, and log a warning if it's used as it was already considered legacy 6 years ago.
Additionally, I'm not exactly sure what's needed for rightscale userdata. The context around it makes it sound like it's a value embedded within other yaml, rather being standard yaml itself, but I'm hoping somebody else has some additional context there.
Also, Rightscale and Salt don't even exist as independent entities anymore, but I'm assuming their modules can still be used by those who bought the technology.