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

*: remove support for container linux #221

Merged
merged 2 commits into from
Sep 11, 2018
Merged

*: remove support for container linux #221

merged 2 commits into from
Sep 11, 2018

Conversation

crawford
Copy link
Contributor

@crawford crawford commented Sep 6, 2018

This drops a number of configuration options that are specific to
Container Linux. This also changes the default region to us-east-1
throughout the tests.

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 6, 2018
@wking
Copy link
Member

wking commented Sep 7, 2018

Since there is no lookup mechanism for RHCOS images, an arbitrary AMI is hardcoded...

For a structured approach to this, you may want to cherry-pick 9db8ed55 from #119 (or maybe I should PR it separately, since we could land it before openshift/release#1317?).

Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me :).

# chdir into the assets path directory
cd "$ASSETS_PATH/tectonic"
cd "tectonic"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the service file to launch this script in the asset directory? Then we could drop this cd entirely.

@crawford
Copy link
Contributor Author

crawford commented Sep 7, 2018

For a structured approach to this, you may want to cherry-pick 9db8ed5 from #119

@wking Yeah, I knew that one was coming. I figured we could land this and then integrate with that package in the future.

@wking
Copy link
Member

wking commented Sep 7, 2018

I figured we could land this and then integrate with that package in the future.

I'm not suggesting you pull in all of #119, just 9db8ed5. Then you could fallback to an AMI calculated by that (stub) package if the caller didn't set an override AMI in their config YAML. That gives us better forward compat, because callers can continue to not care which AMI gets used (while with your PR as it stands there will be a window when callers have to care and set a RHCOS AMI).

@crawford
Copy link
Contributor Author

crawford commented Sep 7, 2018

/hold
We need to wait for openshift/release#1317 and #230 before merging.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2018
We currently calculate CoreOS Container Linux AMIs in Terraform
(installer/modules/aws/ami), but with the coming shift to RHCOS and Go
asset generation, it would be nice to calculate them in Go.  This
commit adds a package to do so.  Currently it's enough of a stub to
support testing, with the value based on [1]:

  {
    "HVM":"ami-06d864b4154214132",
    "SnapshotID":"snap-07b63a4c2f8869c15",
    "S3Object":"s3://openshift-qe-images/rhcos/cloud/rhcos-4.0.5122-aws.vmdk"
  }

from [2].  Once we get a public version of [2] (plans in [3]), we can
replace the stub in this package with something that works for more
regions and channels.

[1]: openshift/release#1344 (comment)
[2]: http://aos-ostree.rhev-ci-vms.eng.rdu2.redhat.com/rhcos/images/aws-us-east-1-tested.json
[3]: openshift/release#1344 (comment)
This drops a number of configuration options that are specific to
Container Linux. This also changes the default region to us-east-1
throughout the tests.
@wking
Copy link
Member

wking commented Sep 10, 2018

The smoke error:

* module.vpc.data.aws_route_table.worker[3]: data.aws_route_table.worker.3: Your query returned no results. Please change your search criteria and try again.

is likely a flake, because e2e-aws succeeded.

/retest

@wking
Copy link
Member

wking commented Sep 10, 2018

/lgtm

@wking
Copy link
Member

wking commented Sep 10, 2018

/hold cancel

openshift/release#1317 and related work are done.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2018
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, 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-merge-robot openshift-merge-robot merged commit 905e2d5 into openshift:master Sep 11, 2018
wking added a commit to wking/openshift-release that referenced this pull request Sep 11, 2018
Since openshift/installer@66ef3beb (*: remove support for container
linux, 2018-09-06, openshift/installer#221), the installer defaults to
a recent RHCOS AMI.  Drop the override here, so we can track RHCOS
with only installer-repo bumps.  I've left the variable in the
template so that, if this repo needs to have its own, separate,
opinion again, it can set the override again.  However, to support the
no-local-opinion case, the new local-opinion syntax will be:

  export EC2_AMI_OVERRIDE='ec2AMIOverride: ami-07307c397daf4d02e'
@ashcrow ashcrow mentioned this pull request Sep 11, 2018
2 tasks
wking added a commit to wking/openshift-installer that referenced this pull request Sep 11, 2018
Catching up with 66ef3be (*: remove support for container linux,
2018-09-06, openshift#221).
@crawford crawford deleted the container-linux branch September 11, 2018 21:09
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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants