OCPCLOUD-3320: Use new manifests-gen#259
Conversation
|
@mdbooth: This pull request references OCPCLOUD-3320 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
mdbooth
force-pushed
the
manifests-gen
branch
2 times, most recently
from
17b9bce to
b512242
Compare
|
@mdbooth: This pull request references OCPCLOUD-3320 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
theobarberbany
force-pushed
the
manifests-gen
branch
from
b512242 to
23ede93
Compare
openshift-merge-robot
added
the
needs-rebase
theobarberbany
force-pushed
the
manifests-gen
branch
from
23ede93 to
caa503b
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@mdbooth: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
theobarberbany
force-pushed
the
manifests-gen
branch
from
caa503b to
e191066
Compare
mdbooth
force-pushed
the
manifests-gen
branch
from
e191066 to
4dcf794
Compare
There was a problem hiding this comment.
Have we considered writing multiple files rather than one enormous, unreviewable file?
There was a problem hiding this comment.
This file is not primarily for review. It's for consumption by the installer, and one big file is easier to manage. Also, as the output of kustomize is a single KRM-formatted dooberry, we'd have to invent a scheme to split it on. We'd be adding complexity in the producer in order to create complexity for the consumer.
In terms of reviewability, it's no more or less reviewable than the installer_components.yaml we had for review, but no longer need because this file is now always available. What makes you say it's unreviewable? The vendor directory regularly breaks GH, but that's a separate issue.
There was a problem hiding this comment.
It's a 25k line file?
I would imagine that when we are regenerating files, it would be easier to review if you could see it touched a particular configmap vs some random lines in the midst of 25k
I didn't like that the previous iteration was unreviewable either FWIW
| COPY openshift/manifests /manifests | ||
| COPY openshift/capi-operator-manifests /capi-operator-manifests |
There was a problem hiding this comment.
Why were we previously copying from the builder?
There was a problem hiding this comment.
No idea. It was unnecessary, though.
| -install-order=10 \ | ||
| -attribute version="${PROVIDER_VERSION}" \ | ||
| -self-image-ref registry.ci.openshift.org/openshift:cluster-capi-controllers \ | ||
| -protect-cluster-resource cluster |
There was a problem hiding this comment.
What would this flag accept for other types, e.g. AWSCluster? I'm surprised this is just cluster and not a fully qualified name?
There was a problem hiding this comment.
This is an override for the heuristic which was there before, but which seemed to be tripped up by most providers due to having more than one infracluster type.
We could make this fully qualified. Is it worth respinning for it? It would be annoying to change later, so maybe.
theobarberbany
force-pushed
the
manifests-gen
branch
from
4dcf794 to
6b30219
Compare
theobarberbany
force-pushed
the
manifests-gen
branch
from
6b30219 to
747faa8
Compare
4 participants
Use new manifests-gen
TODO: