-
Notifications
You must be signed in to change notification settings - Fork 13
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
fix osc sdk go v2 migration and pointers #115
Conversation
fbfecb5
to
6365eb9
Compare
6986e13
to
66e6c80
Compare
builder/osc/common/block_device.go
Outdated
@@ -114,12 +114,15 @@ func buildOscBlockDevicesVmCreation(b []BlockDevice) []oscgo.BlockDeviceMappingV | |||
} | |||
|
|||
if blockDevice.SnapshotId != "" { | |||
log.Printf("snapshotId is nt nul ") |
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.
improve log
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.
Typo "not"
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.
done
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.
We need to clarify how we deal with pointers
*device.Bsu.Iops = 0 | ||
device.Bsu.SetIops(0) |
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.
If it is not an iops, do not set IOPS
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.
I would set nil
pointer to the Iops variable
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.
done
@@ -146,16 +147,21 @@ func (s *StepRegisterOMI) combineDevices(snapshotIDs map[string]string) []oscgo. | |||
} | |||
|
|||
func copyToDeviceMappingImage(device osc.BlockDeviceMappingVmCreation) oscgo.BlockDeviceMappingImage { | |||
log.Printf("Copydevice mapping image ") |
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.
typo: "Copy device"
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.
done
Iops: device.Bsu.Iops, | ||
SnapshotId: device.Bsu.SnapshotId, |
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.
I think the previous code was the right thing to do. Just copy what you have. The big problem here is that device
contains invalid data.
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.
done
if device.Bsu.GetSnapshotId() != "" { | ||
deviceImage.Bsu.SetSnapshotId(*oscgo.PtrString(*device.Bsu.SnapshotId)) | ||
} | ||
if device.Bsu.GetIops() != 0 { | ||
deviceImage.Bsu.SetIops(*device.Bsu.Iops) | ||
} |
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.
I would not do that, just copy the pointer
builder/osc/common/block_device.go
Outdated
@@ -114,12 +114,15 @@ func buildOscBlockDevicesVmCreation(b []BlockDevice) []oscgo.BlockDeviceMappingV | |||
} | |||
|
|||
if blockDevice.SnapshotId != "" { | |||
log.Printf("snapshotId is nt nul ") |
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.
Typo "not"
1f66467
to
437286a
Compare
@@ -154,6 +154,7 @@ func (s *StepNetworkInfo) Run(_ context.Context, state multistep.StateBag) multi | |||
} | |||
} | |||
|
|||
ui.Message(fmt.Sprintf("NetId is null '%s'", s.NetId)) |
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.
Sure about that ?
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.
removed
log.Printf("subregion is %s", subregion) | ||
if subregion != "" { | ||
log.Printf("placement subregion is %s", subregion) |
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.
Duplicate log, you can remove one of them
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.
removed
Signed-off-by: outscale_hmi <hanen.mizouni@outscale.com>
Signed-off-by: outscale_hmi <hanen.mizouni@outscale.com>
No description provided.