Skip to content

Conversation

@cgwalters
Copy link
Member

I want to use this for live ISO testing, since being able to debug
on the console is super useful.

Also, this helps drain cmd-run into mantle.

qemuexec is slowly becoming something nontrivial...but most
of that mess is the Ignition conversion bits which are duplicated
around.

This got subtly broken in a refactoring; we need to infer
the ignition version from the disk image name if no cosa build
is provided.

This is necessary for handling `cosa run -d /path/to/rhcos-4.2.0.qcow2`.
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Some minor comments, but LGTM as is too!

ignition = newconfig
cleanupIgnition = true
}
if kola.Options.IgnitionVersion == "v2" {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: just so I understand, this if-statement isn't related to the autologin draining, right? Feels like it's worth its own commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's necessary now because previously cosa run worked with RHCOS, but kola qemuexec required external translation.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a similar version of this down translation code in a few places. Might end up being worthwhile to move more into the Conf / UserData struct and providing a stable interface to perform translation from there.

I want to use this for live ISO testing, since being able to debug
on the console is super useful.

Also, this helps drain `cmd-run` into mantle.

`qemuexec` is slowly becoming something nontrivial...but most
of that mess is the Ignition conversion bits which are duplicated
around.
@jlebon
Copy link
Member

jlebon commented Mar 23, 2020

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 f3ac1a5 into coreos:master Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants