fix(HMS-2757): Do not cascade on pubkey delete #725
Conversation
internal/dao/dao_errors.go
Outdated
| @@ -38,4 +38,7 @@ var ( | |||
|
|
|||
| // ErrReservationRateExceeded is returned when SQL constraint does not allow to insert more reservations | |||
| ErrReservationRateExceeded = usrerr.New(429, "rate limit exceeded", "too many reservations, wait and retry") | |||
|
|
|||
| // ErrPubkeyNotFound is returned when a nil pointer to a pubkey is used for reservation detail | |||
| ErrPubkeyNotFound = usrerr.New(404, "pubkey not found", "no pubkey found") | |||
There was a problem hiding this comment.
Maybe to stress out the fact that this was caused very likely by deletion (the usecase we see quite often):
| ErrPubkeyNotFound = usrerr.New(404, "pubkey not found", "no pubkey found") | |
| ErrPubkeyNotFound = usrerr.New(404, "pubkey not found", "no pubkey found (already deleted?)") |
But I have to admit it looks weird, 404 is a 404 period. So feel free to let this wild idea go :-)
There was a problem hiding this comment.
I have modified the message a bit to put the information out there.
There was a problem hiding this comment.
One more idea, we might want to incorporate a message telling users what can they do to fix this, right?
There was a problem hiding this comment.
I thought it cannot be fixed, but sure that is the intention yes.
|
Thank you for the comments. All of them are addressed, as they were valid. 🎉 |
| @@ -58,7 +58,7 @@ func newGCPReservation() *models.GCPReservation { | |||
| AccountID: 1, | |||
| Status: "Created", | |||
| }, | |||
| PubkeyID: 1, | |||
| PubkeyID: func(i int64) *int64 { return &i }(1), | |||
| @@ -23,6 +23,7 @@ var ( | |||
| ErrBothTypeAndTemplateMissing = errors.New("instance type or launch template not set") | |||
| ErrUnsupportedRegion = errors.New("unknown region/location/zone") | |||
| ErrInvalidNamePattern = errors.New("name pattern is not RFC-1035 compatible") | |||
| ErrPubkeyNotFound = errors.New("no pubkey found, it may have been already deleted") | |||
There was a problem hiding this comment.
For plain Go errors I would keep it simple:
| ErrPubkeyNotFound = errors.New("no pubkey found, it may have been already deleted") | |
| ErrPubkeyNotFound = errors.New("no pubkey found") |
Only in "user message" (above) it makes sense I think.
avitova
force-pushed
the
NoCascade
branch
3 times, most recently
from
1ac1996 to
153aed0
Compare
|
/retest |
|
That should be resolved now. /retest |
|
/retest I suspect the gcp is hitting public IP quota and we are then not having public IP present: https://ci.int.devshift.net/job/RHEnVision-provisioning-backend-pr-check/1975/ |
|
Maybe if we expose private IPs to the UI, we can not rely on public IP at all. Would that help @akhil-jha ? |
|
Ah, I guess I've caused conflict here :( Sorry! :( |
| @@ -121,7 +121,7 @@ type GCPReservationResponse struct { | |||
| ID int64 `json:"reservation_id" yaml:"reservation_id"` | |||
|
|
|||
| // Pubkey ID. | |||
| PubkeyID int64 `json:"pubkey_id" yaml:"pubkey_id"` | |||
| PubkeyID *int64 `json:"pubkey_id,omitempty" yaml:"pubkey_id"` | |||
|
Updated the yaml omitempty fields and rebased. |
|
Thank you @avitova ! 🧡 |
So I think this should do. I forgot to set pubkey_id nullable and could not find rollback, so I have two migrations.
Tested scenario: create key, create a reservation with the key, delete the key, GetReservationById.
In this sequence of steps, this all works as expected; we do not have any pubkey id, and GetReservationById returns a response with no pubkey_id. All occurrences of PubkeyId for both responses and pubkey models should be covered by checking for nil pointers, and we should keep in mind, that we do allow empty pointers here.