Refactor determination of storageclass for deployments #12042
Conversation
Signed-off-by: Coady LaCroix <[email protected]>
clacroix12
added
Needs Deployment Testing
clacroix12
changed the title
clacroix12
added
Verified
| DEFAULT_STORAGE_CLASS_MAP = { | ||
| constants.AWS_PLATFORM: "gp2-csi", | ||
| constants.IBMCLOUD_PLATFORM: "ibmc-vpc-block-10iops-tier", | ||
| constants.VSPHERE_PLATFORM: "thin-csi", | ||
| constants.AZURE_PLATFORM: "managed-csi", | ||
| constants.GCP_PLATFORM: None, | ||
| constants.ROSA_HCP_PLATFORM: None, | ||
| constants.RHV_PLATFORM: "ovirt-csi-sc", | ||
| } |
There was a problem hiding this comment.
The thin-csi value is also available as constants.THIN_CSI_STORAGECLASS, but I'm not sure, if it is worth to change all the values and get them from constants, so maybe as it is now is ok.
There was a problem hiding this comment.
I noticed that as well and came to the same conclusion. I didn't think it was necessary to convert them each to their own constant. If you think it's worth it to move the map to the constants module I can do that, but for now I will leave it as is.
| "customized_deployment_storage_class" | ||
| ) | ||
|
|
||
| storage_class = DEFAULT_STORAGE_CLASS_MAP.get(platform) |
There was a problem hiding this comment.
Wouldn't be better to use DEFAULT_STORAGE_CLASS_MAP[platform], so it will directly raise a clear KeyError: exception in case we will implement new platform or is None a reasonable "default"?
In the case of raising KeyError exception for missing platform, it might be worth to combine this with the following condition for customized_deployment_storage_class, so in case the platform will be missing in the map, but customized_deployment_storage_class will be defined, it will not raise the exception.
There was a problem hiding this comment.
I originally had it this way but went with .get due to failures in test_cloud.py. I could revert back to using a direct call to the key and change the test class to just set the proper attribute so it doesn't hit any issues.
For the customized_deployment_storage_class case I did add keys for the platforms which leverage this but I do think it's a good idea to check both before raising the error so I will incorporate that as well.
There was a problem hiding this comment.
I moved the check for customized_deployment_storage_class above setting the default value so we will check if that exists as a priority, and changed getting the default value to directly access the key.
ocs_ci/deployment/vmware.py
Outdated
| @@ -129,6 +118,11 @@ def __init__(self): | |||
| self.cluster_launcer_repo_path = os.path.join( | |||
| constants.EXTERNAL_DIR, "v4-scaleup" | |||
| ) | |||
| if version.get_semantic_ocp_version_from_config() >= version.VERSION_4_13: | |||
There was a problem hiding this comment.
You can probably remove this condition and run the following code directly as this condition will be always evaluated to true for the supported versions.
|
Daniel had few valuable comments to consider, but I am also OK with this change - but if you will incorporate Daniel's suggestions I will re-approve. |
Signed-off-by: Coady LaCroix <[email protected]>
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: clacroix12 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 similar comments
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: clacroix12 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: clacroix12 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
A small change here that I want to mention: The default platform in our default_config is now |
Issue: https://url.corp.redhat.com/ce3ae0e