Remove uses of yaml-patch #2212
Conversation
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lleshchi 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 |
openshift-ci
bot
added
the
approved
|
@lleshchi: 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2212 +/- ##
==========================================
- Coverage 58.31% 58.25% -0.07%
==========================================
Files 182 182
Lines 25697 25662 -35
==========================================
- Hits 14986 14950 -36
+ Misses 9453 9447 -6
- Partials 1258 1265 +7
|
| @@ -419,17 +419,21 @@ func (o *Builder) generateInstallConfigSecret() (*corev1.Secret, error) { | |||
| // Remove metadataService field from machinepool platform within installconfig. | |||
| // TODO: Remove this once https://bugzilla.redhat.com/show_bug.cgi?id=2098299 has been addressed. | |||
There was a problem hiding this comment.
I think this bug was fixed in 4.11, so we may be able to get rid of this hackery altogether.
| return nil, errors.Wrap(err, "error removing metadataService field from install-config.yaml") | ||
| } | ||
| d, err = sjson.DeleteBytes(d, `controlPlane.platform.aws.metadataService`) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "error removing metadataService field from install-config.yaml") |
There was a problem hiding this comment.
These error messages should be distinguishable so we know which path broke.
| return nil, errors.Wrap(err, "error removing metadataService field from install-config.yaml") | |
| } | |
| d, err = sjson.DeleteBytes(d, `controlPlane.platform.aws.metadataService`) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "error removing metadataService field from install-config.yaml") | |
| return nil, errors.Wrap(err, "error removing metadataService field from compute section of install-config.yaml") | |
| } | |
| d, err = sjson.DeleteBytes(d, `controlPlane.platform.aws.metadataService`) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "error removing metadataService field from controlPlane section of install-config.yaml") |
| return nil, errors.Wrap(err, "error removing osImage field from install-config.yaml") | ||
| } | ||
| d, err = sjson.DeleteBytes(d, `controlPlane.platform.azure.osImage`) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "error removing osImage field from install-config.yaml") |
There was a problem hiding this comment.
Likewise
| return nil, errors.Wrap(err, "error removing osImage field from install-config.yaml") | |
| } | |
| d, err = sjson.DeleteBytes(d, `controlPlane.platform.azure.osImage`) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "error removing osImage field from install-config.yaml") | |
| return nil, errors.Wrap(err, "error removing osImage field from compute section of install-config.yaml") | |
| } | |
| d, err = sjson.DeleteBytes(d, `controlPlane.platform.azure.osImage`) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "error removing osImage field from controlPlane section of install-config.yaml") |
| @@ -853,7 +851,7 @@ func (m *InstallManager) generateAssets(cd *hivev1.ClusterDeployment, mapPoolsBy | |||
| // For details, see HIVE-1802. | |||
| // Configure Hive MachineSet manifests with a label for the MachinePool name | |||
| var awsClient awsclient.Client | |||
| var vpcID string | |||
| vpcID := "" | |||
| addSecurityGroup := (workerMachinePool != nil) && (metav1.HasAnnotation(workerMachinePool.ObjectMeta, constants.ExtraWorkerSecurityGroupAnnotation)) | |||
There was a problem hiding this comment.
nit: Seems like we could get rid of the addSecurityGroup bool and just use the non-empty-ness of vpcID to indicate same. It would be one less arg to pass into the already-cluttered patchMachineSetManifest func.
Similarly, I feel like we could get rid of addMPLabel. Inline len(mapPoolsByType) > 0 on L874 and then just let the exists on L1014 filter from there.
| @@ -952,7 +933,6 @@ func (m *InstallManager) generateAssets(cd *hivev1.ClusterDeployment, mapPoolsBy | |||
| return nil | |||
| } | |||
|
|
|||
| // TODO: Combine this with patchWorkerMachineSetManifest once HIVE-2397 is completed | |||
| func writeModifiedBytesToFile(modifiedManifestBytes []byte, d fs.DirEntry, path string, logger log.FieldLogger) error { | |||
| if modifiedManifestBytes != nil { | |||
There was a problem hiding this comment.
nit: could invert this condition with a return and save some indent.
| // MachineSet manifest | ||
| securityGroupFilterValue := pool.Annotations[constants.ExtraWorkerSecurityGroupAnnotation] | ||
|
|
||
| manifestBytesJson, err = sjson.SetBytes(manifestBytesJson, `spec.template.spec.providerSpec.value.securityGroups.0.filters.0.values.0`, securityGroupFilterValue) |
There was a problem hiding this comment.
Eh, I think this may be wrong: it is replacing the existing value, whereas the original code is adding.
We start with
securityGroups:
- filters:
- name: tag:Name
values:
- test-czcpt-worker-sgand we want this line to result in
securityGroups:
- filters:
- name: tag:Name
values:
- test-czcpt-worker-sg
- value-from-annotationbut I believe it's instead ending up as
securityGroups:
- filters:
- name: tag:Name
values:
- value-from-annotationNeed to confirm via UT. Existing tests are not sufficient: these additions (which are part of the needed rebase) get partway there, but per this comment we still need to add a check for the above.
This seems to do the trick:
| manifestBytesJson, err = sjson.SetBytes(manifestBytesJson, `spec.template.spec.providerSpec.value.securityGroups.0.filters.0.values.0`, securityGroupFilterValue) | |
| manifestBytesJson, err = sjson.SetBytes(manifestBytesJson, `spec.template.spec.providerSpec.value.securityGroups.0.filters.0.values.-1`, securityGroupFilterValue) |
|
PR needs rebase. 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. |
openshift-merge-robot
added
the
needs-rebase
The yaml-patch package is now abandoned. Replaced with gjson and sjson in appropriate places.
HIVE-2397