Skip to content

Conversation

@dulek
Copy link

@dulek dulek commented Apr 6, 2022

Update from kubernetes-sigs/cluster-api-provider-openstack.

lentzi90 and others added 12 commits March 29, 2022 11:46
The controller test suite was disabled due to missing kubebuilder
assets. These are now installed before running the tests.
A simple sanity check test is also added to confirm that envTest is
working.
…ontroller-tests

🏃 Enable controller tests
🏃 Explain mutability of bastion configuration in CRD
Common network and security group handling between CreateBastion() and
CreateInstance().

A principal advantage of this refactor is that it makes the marshalling
of OpenStackMachineSpec and Instance respectively into an InstanceSpec a
cheap operation which makes no API calls.
Refactor instance creation in machine controller and cluster
controller (for the bastion) to call compute.CreateInstance() with an
InstanceSpec.
…ruct

⚠️ move loadbalancer opts to struct
✨Refactor CreateInstance and CreateBastion
@dulek dulek changed the title Main Update from kubernetes-sigs/cluster-api-provider-openstack Apr 6, 2022
@openshift-ci openshift-ci bot requested review from EmilienM and stephenfin April 6, 2022 15:40
@mdbooth
Copy link

mdbooth commented Apr 6, 2022

/skip ci/prow/unit

@pierreprinetti
Copy link
Member

/retest

@mdbooth
Copy link

mdbooth commented Apr 7, 2022

@mandre Please could you force-merge or override this test failure?

@mdbooth
Copy link

mdbooth commented Apr 7, 2022

For context, the unit tests are failing because the merge pulls in kubernetes-sigs#1183, which enables envtests. This PR made changes to the test target of Makefile to enable these tests. We aren't running this make target, so we don't setup the envtest environment, so the tests fail.

This test failure is just noise and inconvenience right now as:

  • The code was tested upstream
  • The code is tested downstream in MAPO

I understand there was some reason that make test didn't work in downstream prow. My preference would be to disable these tests until we resolve that, otherwise we're potentially going to be chasing our tails keeping these tests working.

@dulek
Copy link
Author

dulek commented Apr 7, 2022

/override ci/prow/unit

Let's see if this works…?

@openshift-ci
Copy link

openshift-ci bot commented Apr 7, 2022

@dulek: dulek unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file.

Details

In response to this:

/override ci/prow/unit

Let's see if this works…?

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.

@pierreprinetti
Copy link
Member

openshift/release#27640

@dulek
Copy link
Author

dulek commented Apr 7, 2022

openshift/release#27640

And it's merged, what now? ;)

@pierreprinetti
Copy link
Member

And it's merged

It's not :P

@mdbooth
Copy link

mdbooth commented Apr 7, 2022

/retest

@openshift-ci
Copy link

openshift-ci bot commented Apr 7, 2022

@dulek: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/unit efbb760 link false /test unit

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@mdbooth
Copy link

mdbooth commented Apr 7, 2022

Hmm. How do we make that test no longer required?

@dulek
Copy link
Author

dulek commented Apr 7, 2022

I'd just try /lgtm /approve and see if prow updates it.

@mdbooth
Copy link

mdbooth commented Apr 7, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2022
@mdbooth
Copy link

mdbooth commented Apr 7, 2022

/approve

@openshift-ci
Copy link

openshift-ci bot commented Apr 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2022
@openshift-merge-robot openshift-merge-robot merged commit f8a255d into openshift:main Apr 7, 2022
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants