MGMT-15691: Remove platform type oci #5787
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
size/XL
|
/test edge-unit-test |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5787 +/- ##
==========================================
+ Coverage 67.89% 67.98% +0.09%
==========================================
Files 235 235
Lines 34548 34480 -68
==========================================
- Hits 23455 23441 -14
+ Misses 9008 8976 -32
+ Partials 2085 2063 -22
|
openshift-ci
bot
added
size/XXL
|
/test edge-unit-test |
|
/test edge-lint |
openshift-ci-robot
added
the
jira/valid-reference
|
@adriengentil: This pull request references MGMT-15691 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@adriengentil: This pull request references MGMT-15691 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@adriengentil: This pull request references MGMT-15691 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@adriengentil: This pull request references MGMT-15691 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@adriengentil: This pull request references MGMT-15691 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test edge-lint edge-unit-test |
|
/test edge-lint edge-unit-test subsystem-test |
|
@adriengentil: Overrode contexts on behalf of adriengentil: ci/prow/edge-e2e-oci-assisted, ci/prow/edge-e2e-oci-assisted-4-14 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| if platform.Type != nil && *platform.Type == models.PlatformTypeExternal { | ||
| // We require valid external settings when platform is set to external | ||
| return validateExternalSettingsForCreate(*platform) | ||
| err = validateExternalSettingsForCreate(*platform) |
There was a problem hiding this comment.
can you write a short description for the methods:
validateExternalPlatform
validateExternalSettingsForCreate - Is it only for the create operation? what about update operation?
areExternalSettingsSet
There was a problem hiding this comment.
we have lots of code that is specific for external platform and oci in /internal/provider/platform.go - can we move it to the external provider?
no, we cannot, it causes import cycles because the provider package is composed of other packages tightly coupled together. I think we should refactor all the packages in provider into one package (platform pakage?), it would also allow to move platform specific code in common/common.go. But, I would do that as part of another ticket/PR.
validateExternalSettingsForCreate - Is it only for the create operation? what about update operation?
We don't need to validate for update because the validations made at swagger level are enough.
I will add some comments 👍
|
/test edge-e2e-oci-assisted-external |
| } | ||
| return nil, errors.New(fmt.Sprintf("For external platform, the platformName must be %s", common.ExternalPlatformNameOci)) |
There was a problem hiding this comment.
The error doesn't look right as we can have any platformName now.
There was a problem hiding this comment.
well, I did not change the behaviour, do you want me to do it? Or you want to do it in a follow-up PR?
There was a problem hiding this comment.
Follow up PR is also fine.
|
/test edge-e2e-oci-assisted-external |
|
/test ci/prow/edge-subsystem-kubeapi-aws |
|
@adriengentil: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test edge-subsystem-kubeapi-aws |
|
/test edge-e2e-oci-assisted-external |
|
Reviewed e2e-metal-assisted-external job: install-config: infrastructure object: |
|
/test edge-e2e-ai-operator-ztp edge-e2e-ai-operator-ztp-capi |
|
@adriengentil: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/override ci/prow/edge-e2e-vsphere-assisted ci/prow/edge-e2e-oci-assisted-4-14 ci/prow/edge-e2e-oci-assisted ci/prow/edge-e2e-nutanix-assisted-4-14 ci/prow/edge-e2e-nutanix-assisted |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, rccrdpccl The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/edge-e2e-vsphere-assisted ci/prow/edge-e2e-oci-assisted-4-14 ci/prow/edge-e2e-oci-assisted ci/prow/edge-e2e-nutanix-assisted-4-14 ci/prow/edge-e2e-nutanix-assisted |
|
@adriengentil: Overrode contexts on behalf of adriengentil: ci/prow/edge-e2e-nutanix-assisted, ci/prow/edge-e2e-nutanix-assisted-4-14, ci/prow/edge-e2e-oci-assisted, ci/prow/edge-e2e-oci-assisted-4-14, ci/prow/edge-e2e-vsphere-assisted In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@adriengentil: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Platformstruct because we needplatform.external.platformNameto return the right external providerPlatformTypeOciandplatform.is_externalflag