Skip to content
This repository has been archived by the owner on Dec 14, 2023. It is now read-only.

Adding KubeVirt Cloud Provider #2

Merged
merged 1 commit into from
Aug 12, 2020
Merged

Conversation

moadqassem
Copy link
Contributor

What this PR does / why we need it:
Adding Kubevirt cloud provider

Release note:

Adding kubevirt initial cloud provider 

@moadqassem moadqassem requested a review from a team June 22, 2020 12:41
@CLAassistant
Copy link

CLAassistant commented Jun 22, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@mfranczy mfranczy left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@gardener-robot
Copy link
Contributor

@mfranczy Command /approve is not known.

@gardener-robot gardener-robot added the reviewed/lgtm Has approval for merging label Jun 23, 2020
Copy link

@mvladev mvladev left a comment

Choose a reason for hiding this comment

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

Some changes are needed.

cmd/machine-controller/main.go Outdated Show resolved Hide resolved
cmd/machine-controller/main.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
kubernetes/crds.yaml Outdated Show resolved Hide resolved
pkg/kubevirt/core/core.go Outdated Show resolved Hide resolved
@moadqassem moadqassem requested a review from a team as a code owner July 17, 2020 13:11
Copy link
Contributor

@stoyanr stoyanr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Please see the comments on the specific lines of code, some of them apply to multiple files, please fix them everywhere. Also, please:

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
pkg/kubevirt/core/core.go Outdated Show resolved Hide resolved
pkg/kubevirt/core/core_test.go Show resolved Hide resolved
pkg/kubevirt/errors/custom_errors.go Show resolved Hide resolved
pkg/kubevirt/plugin.go Show resolved Hide resolved
pkg/kubevirt/plugin.go Show resolved Hide resolved
@stoyanr
Copy link
Contributor

stoyanr commented Aug 10, 2020

@moadqassem Could you also please add install-requirements, format, check-generate, generate, test-cov, test-clean, verify, and verify-extended targets to the Makefile? As an example, see https://github.com/gardener/gardener-extension-provider-kubevirt/blob/master/Makefile. Please also change check and test target to use /vendor/github.com/gardener/gardener/hack. Then, when #4 is merged, we would get a verify check on future PRs.

Copy link
Contributor

@stoyanr stoyanr left a comment

Choose a reason for hiding this comment

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

@moadqassem Thanks for addressing all remaining comments! Please see a new comment that I think is probably a bug.
Reg. my comments about adding additional targets to the Makefile using Gardener hack scripts, that could also be addressed in a future PR.

pkg/kubevirt/core/core.go Outdated Show resolved Hide resolved
@moadqassem
Copy link
Contributor Author

@moadqassem Thanks for addressing all remaining comments! Please see a new comment that I think is probably a bug.
Reg. my comments about adding additional targets to the Makefile using Gardener hack scripts, that could also be addressed in a future PR.

Yeah I will take care of that :-). I also still need to squash my comments and wait for another PR to be merged on the original repo(it is a PR that @mfranczy has opened regarding Dockerfile)

@mfranczy
Copy link
Contributor

@moadqassem the docker file PR has been rebased moadqassem#1

Copy link
Contributor

@stoyanr stoyanr left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the constructor issue! Some more requests:

  • Please add +x flag on .ci/build and .ci/check.
  • Please fix all linter issues:
$ make check
Running go vet...
# github.com/gardener/machine-controller-manager-provider-kubevirt/cmd/machine-controller
cmd/machine-controller/main.go:45:3: Fprint call has possible formatting directive %v
make: *** [check] Error 2
  • Please add a verify target that calls test and check. We should use it for a verify check (until we add verify-extended as suggested previously).

@moadqassem moadqassem force-pushed the master branch 2 times, most recently from 8462228 to 719d82f Compare August 11, 2020 16:08
@stoyanr stoyanr dismissed mvladev’s stale review August 12, 2020 08:54

mvladev is in vacation and his comments have been already addressed

* adding tests for kubevirt cloud provider core module

* adding .ci dir

* injecting the kubevirt client in the cloud provider plugin

* add Dockerfile and build script

* fixing linter and vet issues

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@mfranczy mfranczy merged commit 7e4cabc into gardener-attic:master Aug 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
reviewed/lgtm Has approval for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants