NO-JIRA: Isolate manifest handling in release-extraction #2048
Conversation
hongkailiu
changed the title
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hongkailiu 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 |
hongkailiu
force-pushed
the
OTA-1010-simplify-2
branch
2 times, most recently
from
68470c8 to
e144a77
Compare
hongkailiu
changed the title
|
Test e144a77 with
$ ./oc adm release extract --included --credentials-requests --to credentials-requests quay.io/openshift-release-dev/ocp-release:4.14.0-rc.0-x86_64
Extracted release payload from digest sha256:1d2cc38cbd94c532dc822ff793f46b23a93b76b400f7d92b13c1e1da042c88fe created at 2023-09-07T07:37:47Z
➜ oc git:(OTA-1010-simplify-2) ✗ rg ImageRegistry credentials-requests
credentials-requests/0000_50_cluster-image-registry-operator_01-registry-credentials-request.yaml
6: capability.openshift.io/name: ImageRegistry
➜ oc git:(OTA-1010-simplify-2) ✗ ll credentials-requests
total 48
-rw-r--r--@ 1 hongkliu staff 1.8K Jun 20 15:08 0000_30_machine-api-operator_00_credentials-request.yaml
-rw-r--r--@ 1 hongkliu staff 738B Jun 20 15:08 0000_50_cloud-credential-operator_05-iam-ro-credentialsrequest.yaml
-rw-r--r--@ 1 hongkliu staff 1.3K Jun 20 15:08 0000_50_cluster-image-registry-operator_01-registry-credentials-request.yaml
-rw-r--r--@ 1 hongkliu staff 920B Jun 20 15:08 0000_50_cluster-ingress-operator_00-ingress-credentials-request.yaml
-rw-r--r--@ 1 hongkliu staff 1.0K Jun 20 15:08 0000_50_cluster-network-operator_02-cncc-credentials.yaml
-rw-r--r--@ 1 hongkliu staff 1.5K Jun 20 15:08 0000_50_cluster-storage-operator_03_credentials_request_aws.yaml
$ ./oc adm release extract --included --file=0000_30_machine-api-operator_00_credentials-request.yaml quay.io/openshift-release-dev/ocp-release:4.14.0-rc.0-x86_64
apiVersion: cloudcredential.openshift.io/v1
kind: CredentialsRequest
... |
|
/cc |
hongkailiu
force-pushed
the
OTA-1010-simplify-2
branch
from
e144a77 to
077ae8f
Compare
This pull adds a new type `ManifestReceiver` that works as a middleware between the upstream `extract.ExtractOptions`'s `TarEntryCallback` and the downstream `manifestsCallback`. There are two files `image-references` and `release-metadata` that are handled differently from manifest files. When those files come, their readers from the upstream are sent to the downstream callback right away. Other files contain manifests. They are parsed out and then sent to the downstream. We will embed more changes into this part, e.g., collecting all manifests in the image and then use them to calculate the enabled capabilities which are sent as an argument to the downstream callback. Those changes are coming in other pulls.
hongkailiu
force-pushed
the
OTA-1010-simplify-2
branch
from
077ae8f to
9944151
Compare
hongkailiu
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request explicitly references no jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
| // * with the reader that contains the content of each file whose name is in skipNames. | ||
| // All the errors encountered when parsing the manifests are collected in ManifestErrs. | ||
| type ManifestReceiver struct { | ||
| manifestsCallback func(filename string, manifests []manifest.Manifest, reader io.Reader) (cont bool, err error) |
There was a problem hiding this comment.
Maybe just call it downstreamCallback? Even your Godoc talks about it that way. Calling it manifestCallback is a bit confusing to me because it kinda hints that it would be called with just manifests (esp. together with the other field called 'skipNames`)
| // All the errors encountered when parsing the manifests are collected in ManifestErrs. | ||
| type ManifestReceiver struct { | ||
| manifestsCallback func(filename string, manifests []manifest.Manifest, reader io.Reader) (cont bool, err error) | ||
| skipNames sets.Set[string] |
There was a problem hiding this comment.
The skipNames naming is a bit confusing to me; "skip" makes me think they are skipped entirely, but they are actually just not parsed as manifests. And actually they are anti-skipped because they would otherwise be skipped because they are not json/yaml files.
| klog.V(4).Infof("Found manifest %s", h.Name) | ||
| ms, err := manifest.ParseManifests(r) | ||
| if err != nil { | ||
| mr.ManifestErrs = append(mr.ManifestErrs, errors.Wrapf(err, "error parsing %s", h.Name)) |
There was a problem hiding this comment.
This makes the callback concurrent-unsafe; that's fine (IIRC you already checked once that it is never called in parallel - but maybe it should?) but the GoDoc should mention it.
|
@hongkailiu: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
This pull adds a new type
ManifestReceiverthat works as a middlewarebetween the upstream
extract.ExtractOptions'sTarEntryCallbackandthe downstream
manifestsCallback.There are two files
image-referencesandrelease-metadatathat arehandled differently from manifest files. When those files come, their
readers from the upstream are sent to the downstream callback right
away.
Other files contain manifests. They are parsed out and then sent to the
downstream. We will embed more changes into this part, e.g., collecting
all manifests in the image and then use them to calculate the enabled
capabilities which are sent as an argument to the downstream callback.
Those changes are coming in other pulls.
/hold
Will rebase after #2047 gets in.