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
Add infra to skip inapplicable modules #1597
Add infra to skip inapplicable modules #1597
Conversation
a2f7c2f
to
5b82d30
Compare
5b82d30
to
bfa2f80
Compare
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.
@aciba90 looks really good and behaves well. A couple of minor comments, one related to moving the importlib changes out of this PR if we can.
I think there could be a possibility that we may not want to continue to do the initial "skip inapplicable" modules check if cloud-config instance-data hasn't changed. But, that is a minor recurring cost per boot during module discovery. It may be nice at some point if we grew the ability to report module status from cloud-init status --long [--format json]
that could report whether the module participated in boot, or was skipped (either due to previously running or due to inactivity/inapplicable config). This is not something for this PR, but future feature question for us to discuss long-term.
medium term, maybe for separate PR as well: the write_files vs write_files_deferred case is potentially interesting. If we have write_files: defered: true
in config we could skip running the base cc_write_files
. This is the only case like this were specific values of config key indicates whether or not to skip the module. But, I think it's a corner case that may not be worth investing time in.
cloudinit/config/schema.py
Outdated
if not meta.get("skip_by_schema"): | ||
return "" | ||
schema_keys = ", ".join(meta["skip_by_schema"]) | ||
return f"**Skipped if keys not present:** {schema_keys}\n\n" |
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.
😄 on doc updates.
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.
Updated to:
return f"**Activate only if given keys:** {schema_keys}\n\n"
bfa2f80
to
58e89c7
Compare
Extracted to #1605
That would be interesting. Added to SC-1192 to not forget about its discussion.
If this is the only corner case, I would say that it is not worth to implement a more complex way to express logic within {
"activate_by_schema_keys": ["write_files.defered == True"]
} But if this necessity grows, for sure we could do it. Thanks, @blackboxsw, for the review and constructive feedback! |
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.
Looks really good. one minor rendered text change request (which I realize affects your unit tests). Otherwise +1 from me. I'm testing this now
cloudinit/config/schema.py
Outdated
schema_keys = ", ".join(meta["activate_by_schema_keys"]) | ||
return f"**Activate only if given keys:** {schema_keys}\n\n" |
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.
Let's mark up the key names with ``<key_name>`` to align with other doc config key references. It makes readthedocs decorate it in red.
schema_keys = ", ".join(meta["activate_by_schema_keys"]) | |
return f"**Activate only if given keys:** {schema_keys}\n\n" | |
schema_keys = ", ".join( | |
f"``{k}``" for k in meta["activate_by_schema_keys"] | |
) | |
return f"**Activate only on keys:** {schema_keys}\n\n" |
I think the commit message needs to be updated, but otherwise looks good to me! |
Pushed the minor doc change and will land when CI passes |
Thanks, @blackboxsw and @holmanb, for handling and landing this. |
Proposed Commit Message
Additional Context
SC-1132
Original draft PR: #1515
Test Steps
Checklist: