-
Notifications
You must be signed in to change notification settings - Fork 19
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
Extend nic with mac #240
Conversation
Hello @Vatson112 thank you very much for this improvement. Could you please add tests for your code? |
Hello @janosdebugs! I added 2 tests: for mac address creation and mac address validation. |
There was a problem hiding this 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?
@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 |
Hi @engelmi! I cannot solve the problem with Running > 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 > ~/go/bin/gci diff --skip-generated util_wait_for_job_finished.go Also I tried > ~/go/bin/goimports -d util_wait_for_job_finished.go So, I need your help to understand how to fix this. |
Hi @Vatson112, gofmt -w newmock.go new.go util_wait_for_job_finished.go should suffice. (I think these are the files the linter is failing) |
Hi! Nope, > gofmt -d util_wait_for_job_finished.go
> gofmt -d newmock.go
> gofmt -d new.go > gofmt -l . |
Interesting, locally this did fix it for me - its just about formatting the doc comments. |
I use I think I fixed the issue with Firstly I tried to use docker container with Then, I found golangci-lint run -c .golangci.yml --whole-files --fix I'll write commit now. And we will check CI. |
There was a problem hiding this 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 :)
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]>
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
...