OTA-994: pkg/cli/admin/release/extract: Centralize manifest extraction #1404
Conversation
openshift-ci
bot
added
the
approved
wking
force-pushed
the
unify-release-extract-cases
branch
4 times, most recently
from
9cd8a49
to
261e686
Compare
|
/cc |
|
/cc |
wking
force-pushed
the
unify-release-extract-cases
branch
2 times, most recently
from
e125e01
to
881647b
Compare
wking
force-pushed
the
unify-release-extract-cases
branch
from
881647b
to
6e7d860
Compare
|
|
wking
force-pushed
the
unify-release-extract-cases
branch
2 times, most recently
from
1e0d0d7
to
b67e995
Compare
|
/hold This needs some more work to cover the usual file extraction... |
openshift-ci
bot
added
the
do-not-merge/hold
wking
force-pushed
the
unify-release-extract-cases
branch
from
b67e995
to
2f128c0
Compare
|
Ok, I think I've got the various cases working with 2f128c0. /hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
wking
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@wking: This pull request references OTA-994 which is a valid 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 kubernetes/test-infra repository. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
| continue | ||
| } | ||
| if expectedProviderSpecKind != "" { |
There was a problem hiding this comment.
maybe we could simplify the conditions in the loop and avoid multiple continue
for _, m := range ms {
if o.CredentialsRequests && m.GVK == credentialsRequestGVK && expectedProviderSpecKind != "" {
kind, _, err := unstructured.NestedString(m.Obj.Object, "spec", "providerSpec", "kind")
if err != nil {
return false, errors.Wrap(err, "error extracting cred request kind")
}
if kind == expectedProviderSpecKind {
manifestsToWrite = append(manifestsToWrite, m)
}
}
}There was a problem hiding this comment.
I wanted to keep the manifestsToWrite = append(manifestsToWrite, m) outside of the o.CredentialsRequests branch, so I had a single location to add a m.Include(...) call in follow-up work for --included.
|
/test ci/prow/e2e-agnostic-ovn-cmd This one from |
|
@petr-muller: 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 e2e-agnostic-ovn-cmd |
wking
force-pushed
the
unify-release-extract-cases
branch
3 times, most recently
from
2cba141
to
b2b34df
Compare
|
Still something going on in that metal presubmit: |
wking
force-pushed
the
unify-release-extract-cases
branch
2 times, most recently
from
dfb3937
to
05dd9ca
Compare
De-duplicate the manifest-walking logic that used to be split between the ExtractManifests and CredentialsRequests branches. The content written to disk (or o.Out) still depends on which of those two is set, but the manifest parsing portion is now shared from a common location. This also shifts us to using a tar-entry callback when extracting manifests, where we used to rely on Mappings. That sets us up for centralized filtering of which manifests to extract to the directory. I've also added some logic to avoid writing to the directory when Directory is the empty string, which allows the mirror command to continue to consume the full set of manifests in memory without writing any of them to disk.
wking
force-pushed
the
unify-release-extract-cases
branch
from
05dd9ca
to
29abd12
Compare
|
Ok, seems like b2b34df -> 29abd12 fixes the |
|
@wking: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LalatenduMohanty, petr-muller, wking 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 |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
De-duplicate the manifest-walking logic that used to be split between the
ExtractManifestsandCredentialsRequestsbranches. The content written to disk (oro.Out) still depends on which of those two is set, but the manifest parsing portion is now shared from a common location.This continues the effort begun in #1237 now that openshift/origin#27822 is in place to protect me from accidentally breaking things again. The eventual goal is to only extract expected-to-be-reconciled manifests, but I don't want to need to land that logic in two places, and this should be a no-op refactor that will allow me to land it in one place.