[DevSetting] Replace cinc_version/cinc_installer_url with CliAttributeOverrides which is similar to ExtraChefAttributes#7447
Conversation
himani2411
added
skip-changelog-update
…eOverrides JSON * Model the CLI-side CINC overrides as a single freeform JSON DevSetting (CliAttributeOverrides), mirroring Cookbook.ExtraChefAttributes, instead of two scalar settings. The config exposes cinc_version/cinc_installer_url as read-only accessors that unfurl the blob, so imagebuilder_stack.py and parallelcluster.yaml keep populating CfnParamCincVersion/CfnParamCincInstaller unchanged. * Add CliAttributeOverridesValidator for JSON well-formedness (object), with a KNOWN_CLI_ATTRIBUTE_OVERRIDES hook for optional future strict key validation. * Add tests which cover CliAttributeOverridesValidator (valid object, empty/None, malformed JSON, non-object array/scalar) and the ImagebuilderDevSettings cinc_version/ cinc_installer_url accessors that unfurl the JSON blob (full, empty, partial, unknown-keys).
himani2411
force-pushed
the
cli-devsetting
branch
from
1853621 to
35b460b
Compare
| # ---------------------- Dev Settings Schema ---------------------- # | ||
|
|
||
|
|
||
| class ImagebuilderDevSettingsSchema(BaseDevSettingsSchema): |
There was a problem hiding this comment.
NOTE FOR REVIEWER: The cli_attribute_overrides can later be moved to BaseDevSettingsSchema when we add more overrides; since the 2 paramters are related to only Build Image I am keeping it in ImagebuilderDevSettingsSchema
| f"Invalid value in {CLI_ATTRIBUTE_OVERRIDES_PATH}: must be a JSON object.", | ||
| FailureLevel.ERROR, | ||
| ) | ||
| # TODO: optionally validate keys against KNOWN_CLI_ATTRIBUTE_OVERRIDES here. |
There was a problem hiding this comment.
NOTE FOR REVIEWER: Keeping this as a future improvement since we do not validate all JSON content for ExtraChefAttributes as of today and the cinc attributes that I am replacing also do no have any validators. Any un-related attribute apart from the cinc* one's in the JSON will be a no-op.
… cluster all-validators check test_slurm_all_validators_are_called asserts every dev_settings validator fires during cluster validation. CliAttributeOverridesValidator is registered only on the image-builder config, so skip it like the other non-cluster validators.
| CLI_ATTRIBUTE_OVERRIDES_PATH = "DevSettings/CliAttributeOverrides" | ||
| # Keys the CLI currently unfurls from the blob. Kept for an optional future | ||
| # strict check; today validation is well-formedness only (like ExtraChefAttributes). | ||
| KNOWN_CLI_ATTRIBUTE_OVERRIDES = {"cinc_version", "cinc_installer_url"} |
There was a problem hiding this comment.
I guess we can remove this constant because we are only interested in enforcing the json formatting of the CliAttributeOverrides without making eny assumption or enforcement about its field.
In Cookbook Chef Attribute we introduced the validation of specific attributes only because we decided to vend those attributes publicly to mitigate an impact, but in general we should not do it and keep it free form.
| self.distribution_configuration = distribution_configuration | ||
| self.terminate_instance_on_failure = Resource.init_param(terminate_instance_on_failure, default=True) | ||
| self.disable_validate_and_test = Resource.init_param(disable_validate_and_test, default=True) | ||
| self.cinc_installer_url = Resource.init_param(cinc_installer_url, default="") |
There was a problem hiding this comment.
Removing this parameter from DevSettings schema is ok because we do not guarantee backward compatibility for DevSettings by definition. However, I suggest to document the removal in the changelog. the fact that we do not guarantee backward compatibility means it is ok to make this change, but does not mean we should not surface it.
There was a problem hiding this comment.
IF we never surface the addition of DevSettings in chnagelog; why should we surface their removal?
2 participants
Description of changes
Replace cinc_version/cinc_installer_url with CliAttributeOverrides
Model the CLI-side CINC overrides as a single freeform JSON DevSetting (CliAttributeOverrides), mirroring Cookbook.ExtraChefAttributes, instead of two scalar settings. The config exposes cinc_version/cinc_installer_url as read-only accessors that unfurl the blob, so imagebuilder_stack.py and parallelcluster.yaml keep populating CfnParamCincVersion/CfnParamCincInstaller unchanged.
Add CliAttributeOverridesValidator for JSON well-formedness (object), with a KNOWN_CLI_ATTRIBUTE_OVERRIDES hook for optional future strict key validation.
Add tests which cover CliAttributeOverridesValidator (valid object, empty/None, malformed JSON, non-object array/scalar) and the ImagebuilderDevSettings cinc_version/ cinc_installer_url accessors that unfurl the JSON blob (full, empty, partial, unknown-keys).
CincVersion was added [DevSetting] Add CincVersion override to version during imagebuilder #7420
Tests
References
Checklist
developadd the branch name as prefix in the PR title (e.g.[release-3.6]).Please review the guidelines for contributing and Pull Request Instructions.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.