Skip to content
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

cluster-launch-installer-e2e: Start setting expirationDate again #1761

Merged

Conversation

wking
Copy link
Member

@wking wking commented Sep 29, 2018

We'd dropped this in 3f2f01c (#1677). But since openshift/installer@98a35316 (openshift/installer#364), the installer supports setting it again, so we can add it back.

As the name suggests, this variable is not a stable API. Soon the new installer will be able to load the configuration from YAML (again), but we can keep adjusting as the installer evolves.

This pull request also:

Details on any of those in their commit messages.

Fixes #1601 (although I don't go with the "allow caller to only update specific org/repos" approach because of the OWNERS_ALIASES issues discussed in df16e34).

/assign @stevekuznetsov

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/azure Categorizes item related to Azure jobs labels Sep 29, 2018
@stevekuznetsov
Copy link
Contributor

@wking FYI the less green your size/foo label is, the less fun it is to review :)

@stevekuznetsov
Copy link
Contributor

I would prefer these types of independent changes in separate PRs

cluster/ci/config/prow/plugins.yaml Outdated Show resolved Hide resolved
tools/populate-owners/main.go Outdated Show resolved Hide resolved
tools/populate-owners/main.go Outdated Show resolved Hide resolved
@wking
Copy link
Member Author

wking commented Oct 1, 2018

FYI the less green your size/foo label is, the less fun it is to review :) ... I would prefer these types of independent changes in separate PRs

I've spun off two of the early commits into #1768 and #1769. Once #1769 lands, I'll rebase the pull-over-HTTP commits and file a new PR with those. Once that lands, I'll file an new PR with the cluster-launch-installer-e2e move. And once that lands I'll rebase this one to be just the expirationDate restoration. That's my current plan anyway; if you want the commits here split out into different groupings of PRs, let me know :).

@wking
Copy link
Member Author

wking commented Oct 3, 2018

Ok, once #1792 lands I can rebase this to be just the expirationDate restoration. Orthogonally, at any time after both #1791 and #1792 land we can re-run populate-owners to grant ownership of the new template directory to the installer owners.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2018
We'd dropped this in 3f2f01c
(ci-operator/config/openshift/installer/master: Move to
openshift-install, 2018-09-26, openshift#1677).  But since
openshift/installer@98a35316 (pkg/asset/installconfig: Add
_CI_ONLY_STAY_AWAY_OPENSHIFT_INSTALL_AWS_USER_TAGS, 2018-09-28,
openshift/installer#364), the installer supports setting it again, so
we can add it back.

As the name suggests, this variable is not a stable API.  Soon the new
installer will be able to load the configuration from YAML (again),
but we can keep adjusting as the installer evolves.
@wking wking force-pushed the installer-expiration-date-v2 branch from 25d83ec to b20d571 Compare October 4, 2018 05:03
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2018
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 4, 2018
@wking
Copy link
Member Author

wking commented Oct 4, 2018

Ok, #1768, #1769, and #1792 have landed with the bulk of the lead-in work that was initially part of this PR. #1791 is still in flight, but it is largely orthogonal (I'd included it here to help make future changes like expirationDate self-serve for the installer team, but I'm fine leaving it up to the release team in the meantime). So I've just rebased this onto master with just the expirationDate commit (25d83ec -> b20d571).

@stevekuznetsov
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevekuznetsov, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2018
@openshift-merge-robot openshift-merge-robot merged commit 433c45e into openshift:master Oct 4, 2018
@openshift-ci-robot
Copy link
Contributor

@wking: Updated the prow-job-cluster-launch-installer-e2e configmap using the following files:

  • key cluster-launch-installer-e2e.yaml using file ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml

In response to this:

We'd dropped this in 3f2f01c (#1677). But since openshift/installer@98a35316 (openshift/installer#364), the installer supports setting it again, so we can add it back.

As the name suggests, this variable is not a stable API. Soon the new installer will be able to load the configuration from YAML (again), but we can keep adjusting as the installer evolves.

This pull request also:

Details on any of those in their commit messages.

Fixes #1601 (although I don't go with the "allow caller to only update specific org/repos" approach because of the OWNERS_ALIASES issues discussed in df16e34).

/assign @stevekuznetsov

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.

wking added a commit to wking/openshift-release that referenced this pull request Oct 17, 2018
I'd removed these in 3f2f01c
(ci-operator/config/openshift/installer/master: Move to
openshift-install, 2018-09-26, openshift#1677) while removing the shell from
the setup container.  But we got a shell back in b20d571
(cluster-launch-installer-e2e: Start setting expirationDate again,
2018-09-29, openshift#1761), and restoring these handlers will help:

* Trigger early pod wrap-up when the setup container dies.  Both the
  test and teardown containers are monitoring the 'exit' file so they
  can bail early.

* Forward TERM to the installer, so we can exit gracefully when
  someone TERMs the shell script.

For the signal handling to work, we need to keep the shell process
around, so I've dropped the 'exec' from the openshift-install
invocation and moved it to the background instead (so 'jobs -p' will
list it).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. sig/azure Categorizes item related to Azure jobs size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants