Instance import: Use import module for alpha and beta #1497
Conversation
EricEdens
force-pushed
the
ovf-refactor-swap-daisy-workflows
branch
from
e2d9678
to
731414d
Compare
EricEdens
force-pushed
the
ovf-refactor-swap-daisy-workflows
branch
2 times, most recently
from
aa3191d
to
f79dd4c
Compare
EricEdens
force-pushed
the
ovf-refactor-swap-daisy-workflows
branch
2 times, most recently
from
bc074c8
to
730df40
Compare
EricEdens
force-pushed
the
ovf-refactor-swap-daisy-workflows
branch
from
730df40
to
7f76bc7
Compare
| panic(fmt.Sprintf("Invalid image name %q: %v", imageName, err)) | ||
| } | ||
| if err := validation.ValidateProjectID(projectID); err != nil { | ||
| panic(fmt.Sprintf("Invalid projectID %q: %v", projectID, err)) |
There was a problem hiding this comment.
what if the pre-validation logic is not working as expected? In my opinion since it's not a trivial logic, a panic is dangerous, which may prevent cleanup from executing (which I didn't test). logic in "defer" won't be executed AFAIK.
There was a problem hiding this comment.
You have some good points!
- Cleanup: Defers still get executed when there's a panic: https://blog.golang.org/defer-panic-and-recover
- Logging: To ensure that we're notified of the panic, I can trap the panic within
runWithServerLogging[1] and submitlogFailure
How does this sound?
| ) | ||
|
|
||
| // To rebuild mocks, run `go generate ./...` | ||
| //go:generate go run github.com/golang/mock/mockgen -package ovfdomainmocks -source $GOFILE -destination mocks/mock_interfaces.go |
There was a problem hiding this comment.
these 2 lines are a bit confusion to me, which cmd is needed? Or both?
There was a problem hiding this comment.
Good question!
The first ("To rebuild mocks...") is a comment for devs. The second instructs go generate on how to rebuild mocks.
More info:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dntczdx, EricEdens, zoran15 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is a follow-up to #1497 where we refactored instance import to use import modules.
Following this PR, instance imports in beta and alpha will delegate import to code that is shared with
gce_vm_image_import. This enables the following:import_ovfto allow import when it cannot determine the OS. That will come in a future PR)