feat(HMS-2248): Add token support for list template #668
Conversation
adiabramovitch
force-pushed
the
tmp-token
branch
2 times, most recently
from
ca7c350
to
673687e
Compare
| if token == "" { | ||
| logger.Warn().Msg("token is empty, listing first page") | ||
| } | ||
| return context.WithValue(ctx, tokenCtxKey, token) |
There was a problem hiding this comment.
What are the consequences if we add the empty token to the context? Do we want to add the empty string to the context?
There was a problem hiding this comment.
It means that we are listing the first page and we add an empty token to the context
| } | ||
| } | ||
| return &LaunchTemplateListResponse{Data: list} | ||
| return &LaunchTemplateListResponse{Data: list, Metadata: *meta} |
There was a problem hiding this comment.
We might want to check for nil before dereferencing, if that makes sense. For that case, we want to return an empty Metadata structure.
There was a problem hiding this comment.
I dont think it is needed, we are always returning a Metadata struct if you take a look here: NewTokenMetadata and NewOffsetMetadata
there is not a case we return nil from those functions.
There was a problem hiding this comment.
I understand. This was just an idea for better maintaining in the future.
The point is, if someone uses this function in the future, they do not have to know that it does not handle nil. Best functions are the ones of which implementation you do not have to know. 😆
If someone wanted to call the function in the future without NewListLaunchTemplateResponse Metadata parameter, they would have to create an empty one anyway and pass a pointer to such an empty Metadata structure. If this had been done in the codebase more times, it would create a duplication.
In a worse case, they would think that the function accepts a nil pointer. They would find out pretty quickly because of the panic, but yeah, that was just why I thought it might be handy.
Moreover, we pass results from other functions. I.e.: metadata := someFunctionWithPointerReturnValue.
And if someone changes that in the future, because we would want to use a nil pointer as a return value, this breaks as a consequence.
| list := make([]*PubkeyResponse, len(pubkeys)) | ||
| for i, pubkey := range pubkeys { | ||
| list[i] = NewPubkeyResponse(pubkey) | ||
| } | ||
| return &PubkeyListResponse{Data: list, Metadata: info.Metadata, Links: info.Links} | ||
| return &PubkeyListResponse{Data: list, Metadata: *meta} |
| list := make([]*GenericReservationResponse, len(reservations)) | ||
| for i, reservation := range reservations { | ||
| list[i] = reservationResponseMapper(reservation) | ||
| } | ||
| return &GenericReservationListResponse{Data: list, Metadata: info.Metadata, Links: info.Links} | ||
| return &GenericReservationListResponse{Data: list, Metadata: *meta} |
| @@ -42,7 +41,7 @@ func NewListSourcesResponse(sourceList []*clients.Source, info *page.Info) rende | |||
| Status: source.Status, | |||
| } | |||
| } | |||
| return &SourceListResponse{Data: list, Metadata: info.Metadata, Links: info.Links} | |||
| return &SourceListResponse{Data: list, Metadata: *meta} | |||
adiabramovitch
force-pushed
the
tmp-token
branch
from
673687e
to
09b78c7
Compare
|
/retest |
|
@avitova I guess it's up to your final ack now :) |
|
TY all! 🎉 |
Depends on #625
This PR introduces pagination support for AWS and GCP by returning the received token when fetching the corresponding page. To validate the retrieval of AWS pages, the token needs to be escaped since it is in base64 format.
However, tests are currently missing, andI plan on adding those once will get some feedback. Additionally, tests are failing due to the initial commit missing limit and offset. I plan to stub them and proceed with adding the tests once this implementation is agreed upon.
Please let me know if you have any feedback or suggestions regarding this approach before proceeding with the test implementation.
For easy review just take a look at the changes made by the second commit, once the dependent PR will be merged i'll rebase.