-
Notifications
You must be signed in to change notification settings - Fork 55
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
change in private_ipv4 on instance value requires the replacement #255
Conversation
ip, err := apiClient.FindIP(oldReservedIP.(string)) | ||
if err != nil { | ||
if errors.Is(err, civogo.ZeroMatchesError) { | ||
return diag.Errorf("sorry there is no %s IP in your account", oldReservedIP) |
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 suggest to rework and reword this. Something like "Sorry, there is no IP that matches %s in in this network" ?
I suggest "network" rather than account as these are network-specific, right?
if errors.Is(err, civogo.ZeroMatchesError) { | ||
return diag.Errorf("sorry there is no %s IP in your account", oldReservedIP) | ||
} else if errors.Is(err, civogo.MultipleMatchesError) { | ||
return diag.Errorf("sorry we found more than one IP with that value in your account") |
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.
Change this to be "Sorry, more than one IP with that value matches this request" or something?
} else if errors.Is(err, civogo.MultipleMatchesError) { | ||
return diag.Errorf("sorry we found more than one IP with that value in your account") | ||
} else { | ||
return diag.Errorf("error finding IP %s: %s", oldReservedIP, err) |
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.
Capitalize "error"
|
||
_, err = apiClient.UnassignIP(ip.ID, apiClient.Region) | ||
if err != nil { | ||
return diag.Errorf("[ERR] an error occurred while unassigning reserved IP %s from instance %s: %s", ip.ID, instance.ID, err) |
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.
Any reason why this one has "[ERR]" as the prefix to the message and some others don't?
if err != nil { | ||
return diag.Errorf("[ERR] an error occurred while unassigning reserved IP %s from instance %s: %s", ip.ID, instance.ID, err) | ||
} | ||
log.Printf("[INFO] unassigned reserved IP %s from the instance %s", oldReservedIP, d.Id()) |
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.
And this one - why "[INFO]" when other messages are not always prefaced with a classification like this?
if errors.Is(err, civogo.ZeroMatchesError) { | ||
return diag.Errorf("sorry there is no %s IP in your account", newReservedIP) | ||
} else if errors.Is(err, civogo.MultipleMatchesError) { | ||
return diag.Errorf("sorry we found more than one IP with that value in your account") |
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.
See my other comment re multiple matches and rewording that
ip, err := apiClient.FindIP(newReservedIP.(string)) | ||
if err != nil { | ||
if errors.Is(err, civogo.ZeroMatchesError) { | ||
return diag.Errorf("sorry there is no %s IP in your account", newReservedIP) |
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.
Capitalize and add comma: "Sorry, there is no IP matching %s in this network" assuming it's network, not account-specific?
fixes: #247
As it is an immutable field via api, an replacement prompt will occur when change in this value is detected.