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

Extend nic with mac #240

Merged
merged 11 commits into from
Nov 29, 2022
Merged

Extend nic with mac #240

merged 11 commits into from
Nov 29, 2022

Conversation

Vatson112
Copy link
Contributor

Please describe the change you are making

Allow manage and read MAC address of VM NIC interfaces.

Issue reference in terraform client: issue
...

Are you the owner of the code you are sending in, or do you have permission of the owner?

Yes
...

The code will be published under the BSD 3 clause license. Have you read and understood this license?

Yes
...

@ghost
Copy link

ghost commented Nov 7, 2022

Hello @Vatson112 thank you very much for this improvement. Could you please add tests for your code?

@Vatson112
Copy link
Contributor Author

Hello @janosdebugs!

I added 2 tests: for mac address creation and mac address validation.

ghost
ghost previously requested changes Nov 10, 2022
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@Vatson112 thank you very much for your work. However, I don't see any calls to CreateNIC with MAC addresses from test code. Could you please add those tests?

@Vatson112 Vatson112 requested a review from a user November 10, 2022 15:30
@ghost
Copy link

ghost commented Nov 10, 2022

@engelmi can you run live tests on this PR please? The code looks good to me.

@engelmi
Copy link
Contributor

engelmi commented Nov 11, 2022

@engelmi can you run live tests on this PR please? The code looks good to me.

I am on, will take a while, though.

@Vatson112 the linting is currently failing due to gci. This results from using the latest golangci-lint version - and recently v1.50.1 was released. Formatting with goimports should solve this issue. Could you apply it to the affected files?

@Vatson112
Copy link
Contributor Author

Hi @engelmi!

I cannot solve the problem with goimports or gci. And dont understand the problem with golangci-lint.

Running golangci-lint locally I see the problem with 3 files. Take for example 1st.

> golangci-lint run -c .golangci.yml
util_wait_for_job_finished.go:16: File is not `gci`-ed with --skip-generated -s standard,default (gci)
//     correlationID := fmt.Sprintf("image_transfer_%s", utilrand.String(5))
//     conn.
//         SystemService().
//         DisksService().
//         DiskService(diskId).
//         Update().
//         Query("correlation_id", correlationID).
//         Send()
new.go:80: File is not `gci`-ed with --skip-generated -s standard,default (gci)
//   url
new.go:84: File is not `gci`-ed with --skip-generated -s standard,default (gci)
//   username

I tried gci diff to find problem. But it doesnt show anything.

> ~/go/bin/gci  diff  --skip-generated  util_wait_for_job_finished.go 

Also I tried goimports, and It looks the same.

> ~/go/bin/goimports -d  util_wait_for_job_finished.go

So, I need your help to understand how to fix this.

@engelmi
Copy link
Contributor

engelmi commented Nov 14, 2022

Hi @Vatson112,
I confused the tool. It is not goimports, but gofmt - a simple

gofmt  -w newmock.go new.go util_wait_for_job_finished.go

should suffice. (I think these are the files the linter is failing)

@Vatson112
Copy link
Contributor Author

Hi!

Nope, gofmt didn't change files with your command. Also neither show diff, nor list files whose formatting differs from gofmt, show anything.

> gofmt -d util_wait_for_job_finished.go
> gofmt -d newmock.go
> gofmt -d new.go 
> gofmt -l .

@engelmi
Copy link
Contributor

engelmi commented Nov 15, 2022

Interesting, locally this did fix it for me - its just about formatting the doc comments.
Which Go version do you use? My guess would be that this is due to gofmt and doc changes in 1.19 as golangci-lint uses 1.19.

@Vatson112
Copy link
Contributor Author

I use go1.19.2 and golangci-lint version 1.50.1.

I think I fixed the issue with golangci-lint.

Firstly I tried to use docker container with go1.16, but seems it is not a problem with go version.

Then, I found golangci-lint --fix option, so I tried to use it firstly with go1.16 and it was fixed all linter problem, then I tried the same with go1.19 and It also works great.
So my full command is.

golangci-lint  run -c .golangci.yml  --whole-files  --fix

I'll write commit now. And we will check CI.

Copy link
Contributor

@engelmi engelmi left a comment

Choose a reason for hiding this comment

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

Finally got the time (and resources) to test this against a real oVirt engine. Found only one minor issue. As soon as this is fixed, we can finally merge this PR :)

nic_test.go Outdated Show resolved Hide resolved
The additional empty string check on the mac param is required here. If the mac param is not set, a mac for the nic will be generated anyway when testing against a real oVirt engine - and cause this assert func to fail (for example in TestVMNICCreation)

Co-authored-by: Michael Engel <[email protected]>
@engelmi engelmi dismissed ghost ’s stale review November 29, 2022 08:47

requested tests are added

@engelmi engelmi merged commit 855f684 into oVirt:main Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants