[DevSetting] Replace cinc_version/cinc_installer_url with CliAttributeOverrides which is similar to ExtraChefAttributes#7447
Conversation
himani2411
added
skip-changelog-update
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.
| 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?
I propose we do not add this as part of changelog since we never document its existence.
There was a problem hiding this comment.
The point of a changelog entry is to introduce opportunities and anticipate problems.
Even if we have never advertised the param in our changelog, nothing prevents a user from using it.
Such user will face an error. Such error cannot be considered a breaking of a contract because it is an unofficial param, but still the entry will allow the user to avoid/motivate the failure.
If you disagree with that I understand because we are on a gray area.
In this case I suggest to let our PM have a call on this.
himani2411
force-pushed
the
cli-devsetting
branch
from
05addf9 to
7a5b3c0
Compare
…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).
… 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.
himani2411
force-pushed
the
cli-devsetting
branch
from
7a5b3c0 to
3d5adfe
Compare
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.