-
Notifications
You must be signed in to change notification settings - Fork 2.1k
installer: use steps for e2e-{aws,azure,gcp}-shared-vpc #9517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
installer: use steps for e2e-{aws,azure,gcp}-shared-vpc #9517
Conversation
f96d892 to
a7910bf
Compare
a920474 to
8ef94c6
Compare
|
/retest |
| @@ -0,0 +1,11 @@ | |||
| approvers: | |||
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.
should probably include @jstuever and @patrickdillon based on installer AWS approvers. And possibly some folks in your current list aren't interested in being approvers for this specific directory (folks with a general interest in multi-step are probably already approvers from root-wards directories and don't need to be repeated down in this one).
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.
The CI test requires adding OWNERS to each new dir, so i just copied the parent dir owners to this. If we need to change any we should update them in one go. I'm just ensuring the same ownership continues to each child.
|
/retest |
| set -o pipefail | ||
|
|
||
| # TODO: move to image | ||
| curl -L https://github.com/mikefarah/yq/releases/download/3.3.0/yq_linux_amd64 -o /tmp/yq && chmod +x /tmp/yq |
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.
Don't we have an image that contains yq now? Alternative would be to have an image that contains jq and write the install-config in JSON.
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.
we don't have such a image, so for now this should work.
i need to find a good place to install the yq binary.
as for installer and json is not happening any time soon so that doesn't help here.
this should be stable enough to fix in future work.
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.
as for installer and json is not happening any time soon so that doesn't help here.
JSON is a subset of YAML, so the installer will be fine ingesting either.
Looks like openshift/origin#22378 putting jq in test rotted out. But sure, I guess we can curl down unsigned binaries in the short term :p.
| @@ -0,0 +1,10 @@ | |||
| ref: | |||
| as: ipi-conf-aws-sharednetwork | |||
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.
We do this in a few places already, but I hear there are some ci-tools issues with using the same name for both a ref and a chain. Might be worth using different names for them here; or maybe that's no longer an issue.
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.
| @@ -0,0 +1,2 @@ | |||
| approvers: | |||
| - abhinavdahiya | |||
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.
Can we extend this to include @fabianofranz and @jhixson74 ? Having a single approver seems like a risk for over-silo'ed understanding (although root-ward approvers are also theoretically on the hook to provide maintenance in this subdir as well).
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.
Out of band, @abhinavdahiya wants to punt on this too, like here.
|
GCP rehearsal failed on install. AWS rehearsal is strange: But there are no logs from that conf container. Kick them again to see what they do this time: /retest |
|
GCP: Dunno what that's about. |
|
/test pj-rehearse |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, patrickdillon 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 |
|
@abhinavdahiya: Updated the following 5 configmaps:
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 kubernetes/test-infra repository. |
We've been copy/pasting the SHARED_DIR approach around since f249775 (steps: add shared network, 2020-06-05, openshift#9517). But while later steps will need the patched file, they won't need the patch itself. Move the patch into /tmp to save some space and reduce clutter in the shared directory. Generated with: $ sed -i 's|PATCH="${SHARED_DIR}/\([^"]*\)"|PATCH=/tmp/\1|' $(git grep -l 'PATCH=.*SHARED_DIR' ci-operator/step-registry)
…o, PATCH
It's extremely unlikely that there is any content in the target
${PATCH} location before each step writes its specific content. But
just in case, and to help folks reading the code avoid wondering about
it, write the file with '>', instead of appending with '>>'. This
adjusts a pattern we've been following since f249775 (steps: add
shared network, 2020-06-05, openshift#9517).
Generated with:
$ sed -i 's/ >> \("${PATCH}"\)/ > \1/' $(git grep -l '>>.*PATCH' ci-operator/step-registry)
No description provided.