OCPBUGS-25461: Add RHEL9 and RHEL8 based oc as new targets in command extraction #1647
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,11 +58,12 @@ type extractTarget struct { | |
| InjectReleaseVersion bool | ||
| SignMachOBinary bool | ||
|
|
||
| ArchiveFormat string | ||
| AsArchive bool | ||
| AsZip bool | ||
| Readme string | ||
| LinkTo []string | ||
| TargetCommandName string | ||
|
|
||
| Mapping extract.Mapping | ||
| } | ||
|
|
@@ -265,6 +266,30 @@ func (o *ExtractOptions) extractCommand(command string) error { | |
| InjectReleaseVersion: true, | ||
| ArchiveFormat: "openshift-client-linux-amd64-%s.tar.gz", | ||
| }, | ||
| { | ||
| OS: "linux", | ||
| Arch: "amd64", | ||
| Command: "oc.rhel9", | ||
| Mapping: extract.Mapping{Image: "cli-artifacts", From: "usr/share/openshift/linux_amd64/oc.rhel9"}, | ||
|
|
||
| LinkTo: []string{"kubectl"}, | ||
| Readme: readmeCLIUnix, | ||
| InjectReleaseVersion: true, | ||
| ArchiveFormat: "openshift-client-linux-amd64-rhel9-%s.tar.gz", | ||
| TargetCommandName: "oc", | ||
| }, | ||
| { | ||
| OS: "linux", | ||
| Arch: "amd64", | ||
| Command: "oc.rhel8", | ||
| Mapping: extract.Mapping{Image: "cli-artifacts", From: "usr/share/openshift/linux_amd64/oc.rhel8"}, | ||
|
There was a problem hiding this comment. How's the symlink processing implemented here? Will the extracted binary be a symlink or the symlink target? There was a problem hiding this comment. Will the final binary be oc or oc.rhel8? I prefer oc only. There was a problem hiding this comment. Thanks a lot for review.
For which symlink are you asking oc -> kubectl or oc -> oc.rhel8?. If it is for oc -> oc.rhel8, it is the same principle of oc/images/cli-artifacts/Dockerfile.rhel Line 18 in f1cac6d oc/pkg/cli/admin/release/extract_tools.go Lines 257 to 267 in f1cac6d
Good point and it would be nicer, I can change them to There was a problem hiding this comment. @ingvagabund I've added new optional TargetCommandName field to rename extracted binaries. |
||
|
|
||
| LinkTo: []string{"kubectl"}, | ||
| Readme: readmeCLIUnix, | ||
| InjectReleaseVersion: true, | ||
| ArchiveFormat: "openshift-client-linux-amd64-rhel8-%s.tar.gz", | ||
| TargetCommandName: "oc", | ||
| }, | ||
| { | ||
| OS: "linux", | ||
| Arch: "arm64", | ||
|
|
@@ -277,6 +302,32 @@ func (o *ExtractOptions) extractCommand(command string) error { | |
| InjectReleaseVersion: true, | ||
| ArchiveFormat: "openshift-client-linux-arm64-%s.tar.gz", | ||
| }, | ||
| { | ||
| OS: "linux", | ||
| Arch: "arm64", | ||
| Command: "oc.rhel9", | ||
| NewArch: true, | ||
| Mapping: extract.Mapping{Image: "cli-artifacts", From: "usr/share/openshift/linux_arm64/oc.rhel9"}, | ||
|
|
||
| LinkTo: []string{"kubectl"}, | ||
| Readme: readmeCLIUnix, | ||
| InjectReleaseVersion: true, | ||
| ArchiveFormat: "openshift-client-linux-arm64-rhel9-%s.tar.gz", | ||
| TargetCommandName: "oc", | ||
| }, | ||
| { | ||
| OS: "linux", | ||
| Arch: "arm64", | ||
| Command: "oc.rhel8", | ||
| NewArch: true, | ||
| Mapping: extract.Mapping{Image: "cli-artifacts", From: "usr/share/openshift/linux_arm64/oc.rhel8"}, | ||
|
|
||
| LinkTo: []string{"kubectl"}, | ||
| Readme: readmeCLIUnix, | ||
| InjectReleaseVersion: true, | ||
| ArchiveFormat: "openshift-client-linux-arm64-rhel8-%s.tar.gz", | ||
| TargetCommandName: "oc", | ||
| }, | ||
| { | ||
| OS: "windows", | ||
| Arch: "amd64", | ||
|
|
@@ -499,6 +550,7 @@ func (o *ExtractOptions) extractCommand(command string) error { | |
| } | ||
| exactReleaseImage := refExact.String() | ||
|
|
||
| targetArchCommands := make(map[string]struct{}) | ||
| // resolve target image references to their pull specs | ||
| missing := sets.NewString() | ||
| var validTargets []extractTarget | ||
|
|
@@ -513,9 +565,23 @@ func (o *ExtractOptions) extractCommand(command string) error { | |
| continue | ||
| } | ||
| } | ||
|
|
||
| if target.Arch == targetReleaseArch { | ||
| targetArchCommands[target.Command] = struct{}{} | ||
| } | ||
|
|
||
| if target.OS == "linux" && target.Arch == releaseArch { | ||
| if _, ok := targetArchCommands[target.Command]; ok { | ||
| // Some target commands have release-arch types that defines extracting | ||
|
There was a problem hiding this comment. Was this corner case introduced with rhel8/rhel9 change? Or, has it been around for some time already? There was a problem hiding this comment. It has existed for a long time (from this commit 3dba61a), but it didn't cause any problem because all the targets have |
||
| // the command in whatever release architecture type is set(e.g. linux/amd64) | ||
| // But there is also another target type for these commands specifically set to | ||
| // each architecture type(linux/amd64, linux/arm64) and it is expected that | ||
| // one of these arch types collide with release-arch type. Thus, | ||
| // to prevent duplicate extraction, we have to skip the one colliding with release-arch type. | ||
| // However, we need to skip per command name because some command may not have release-arch type. | ||
| klog.V(2).Infof("Skipping duplicate %s", target.ArchiveFormat) | ||
| continue | ||
| } | ||
| } | ||
| spec, err := findImageSpec(release.References, target.Mapping.Image, o.From) | ||
| if err != nil && !target.NewArch { | ||
|
|
@@ -529,6 +595,11 @@ func (o *ExtractOptions) extractCommand(command string) error { | |
| } | ||
| target.Mapping.Image = spec | ||
| target.Mapping.ImageRef = imagesource.TypedImageReference{Ref: ref, Type: imagesource.DestinationRegistry} | ||
| // if the name of the extracted binary is set to different from the | ||
| // actual command name, we set it to new target command name. | ||
| if target.TargetCommandName != "" { | ||
| target.Command = target.TargetCommandName | ||
| } | ||
| if target.AsArchive { | ||
| willArchive = true | ||
| target.Mapping.Name = fmt.Sprintf(target.ArchiveFormat, releaseName) | ||
|
|
@@ -830,6 +901,13 @@ func (o *ExtractOptions) extractCommand(command string) error { | |
| if target.NewArch { | ||
| continue | ||
| } | ||
| if command == "" && (strings.Contains(target.Mapping.From, "rhel9") || strings.Contains(target.Mapping.From, "rhel8")) { | ||
| // if user explicitly wants to extract oc.rhel9(or installer.rhel9) via --command=oc.rhel9 and | ||
| // if release does not have this binary, we can safely return error. | ||
| // On the other hand, if user wants to extract all the tooling in older versions via --tools flag, | ||
| // we shouldn't print any error indicating that oc.rhel9 does not exist in this release payload. | ||
| continue | ||
| } | ||
| missing = append(missing, target.Mapping.From) | ||
| } | ||
| sort.Strings(missing) | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any extract target for ppc64le?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I added it just to support when/if there is a demand to extract oc in ppc64le arch type but I can remove, if it creates confusion.