Skip to content

Conversation

@sallyom
Copy link
Contributor

@sallyom sallyom commented Nov 14, 2018

@stevekuznetsov I think this is required before a PR to add a libvirt test and job to openshift/installer. thanks!

depends on this: openshift/installer#701 for openshift-tests binary, but maybe there's a better way to get that binary available to test container? nm, got it from IMAGE_TESTS

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 14, 2018
@sallyom sallyom force-pushed the add-nested-libvirt-template branch 3 times, most recently from 127db17 to a45bbf7 Compare November 15, 2018 00:55
Copy link
Contributor

Choose a reason for hiding this comment

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

Omit the line number as it's not going to be relevant a few commits later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sallyom sallyom force-pushed the add-nested-libvirt-template branch from a45bbf7 to ff2bcd7 Compare November 15, 2018 18:22
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this so different than other types of testing?

Is your job not intended to run tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this template is to install a libvirt cluster, to test the libvirt setup is not broken, for developers' workflow. This was requested by the installer team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks nodes/workers/router is up for now, will add more tests in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added openshift-tests

Copy link
Contributor

Choose a reason for hiding this comment

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

cluster-launch-installer-libvirt-e2e.yaml

Copy link
Contributor Author

@sallyom sallyom Nov 15, 2018

Choose a reason for hiding this comment

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

ok, done

@sallyom sallyom force-pushed the add-nested-libvirt-template branch from ff2bcd7 to 042bda3 Compare November 15, 2018 18:53
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a relative link?

... configuration file](../cluster/ci/config/prow/plugins.yaml)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave it as/is, bc it goes along with the other links in the doc.

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 drop this trailing empty line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, done

Copy link
Member

Choose a reason for hiding this comment

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

Do you need the - here? I'm not used to seeing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, don't need it, the hyphen is used when appending or prepending typed text to an existing file to create a new file, so that does not belong here, thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it.

@sallyom sallyom force-pushed the add-nested-libvirt-template branch 4 times, most recently from 3a6ed3a to f0b9fe9 Compare November 18, 2018 13:04
Copy link
Contributor Author

@sallyom sallyom Nov 18, 2018

Choose a reason for hiding this comment

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

@smarterclayton I've added openshift-tests binary to the installer-libvirt image, here: openshift/installer@master...sallyom:add-openshift-tests-to-libvirt-ci-image - is there a better/easier way to do get openshift-tests in the test container?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need the binary inside your image, vs just passing a kubeconfig to the test pod? is your gce libvirt cluster not available outside?

Copy link
Contributor

Choose a reason for hiding this comment

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

That change to installer won't work - you can't do that. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nm, I'm taking it from IMAGE_TESTS, got it.

@stevekuznetsov
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sallyom, stevekuznetsov

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2018
@openshift-merge-robot openshift-merge-robot merged commit b458c6b into openshift:master Nov 20, 2018
@openshift-ci-robot
Copy link
Contributor

@sallyom: Updated the plugins configmap using the following files:

  • key plugins.yaml using file cluster/ci/config/prow/plugins.yaml
Details

In response to this:

@stevekuznetsov I think this is required before a PR to add a libvirt test and job to openshift/installer. thanks!

depends on this: openshift/installer#701 for openshift-tests binary, but maybe there's a better way to get that binary available to test container? nm, got it from IMAGE_TESTS

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 Apr 1, 2019
…bvirt-e2e: Drop IMAGE_INSTALLER

This parameter dates back to 128c374 (add installer nested libvirt
template, 2018-11-14, openshift#2143), but has never been used (this template
uses LOCAL_IMAGE_LIBVIRT_INSTALLER instead).
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants