feat(HMS-2248): Add token support for list templates #626

adiabramovitch · 2023-08-02T07:59:32Z

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.

lzap

The same concern.

Now that I see we have two combinations:

limit + offset
limit + token

I think composed type does not make too much sense, checking for nil or better for 0 and/or empty string is also fine.

Still, I recommend to create the functions that will check for nil (in case context does not contain any value) and return 0 or "" in that case so checks are easy.

internal/payloads/launch_template_payload.go

internal/clients/http/gcp/gcp_client.go

akhil-jha · 2023-08-03T09:08:47Z

Launch template test failing. Not able to list the templates.

adiabramovitch · 2023-08-06T10:55:39Z

I will update this PR once #625 will get merged, there are a lot of conflicts but feel free to test it out.

ezr-ondrej

I wonder (and this is really just a thought) if we actually need to do this? Launch templates will usually not be in hundreds and if they do, it does not make sense to paginate in the UI, but search, so I guess we would be fine here not implementing pagination, but only search and some indication of "there is more", but no way of fetching more.
I guess that would quite simplify the whole pagination effort and IMO it would be good enough solution.

lzap · 2023-08-07T15:24:25Z

I wonder (and this is really just a thought) if we actually need to do this? Launch templates will usually not be in hundreds and if they do, it does not make sense to paginate in the UI

I share the sentiment, but I would propose to do at least one pagination for every single type (SQL, sources, AWS, GCP) so we know for sure the design is good. We might not even use it in the UI but we will have backend API tests, documented API and confidence that it works.

akhil-jha · 2023-08-08T09:19:18Z

/retest

ezr-ondrej · 2023-08-09T16:18:09Z

We might not even use it in the UI but we will have backend API tests

My point was, if it would be bit of a struggle to implement (especially if we would want to have same pagination for all endpoints), I'd prefer not to waste the effort, but if we think it's not a big deal, lets do it :)

adiabramovitch · 2023-08-30T11:14:43Z

api/openapi.gen.json

+            "previous": ""
+          },
+          "metadata": {
+            "total": 0


Not sure why it created the total here if it is empty and I am using the omitempty tag.
Wanted to change the metadata struct to pointer but it is the same discussion as on Slack

lzap

I only have naming conventions suggestions. I still do propose the refactoring (single root JSON field) but if you like to keep it this way I just suggest to get rid if the too generic Info (I suggest Metaroot/Metadata).

cmd/spec/path.yaml

lzap · 2023-09-04T07:34:20Z

internal/clients/http/ec2/ec2_client.go

@@ -338,34 +339,37 @@ func (c *ec2Client) DescribeInstanceDetails(ctx context.Context, InstanceIds []s
 	return instanceDetailList, nil
 }

-func (c *ec2Client) ListLaunchTemplates(ctx context.Context) ([]*clients.LaunchTemplate, error) {
+func (c *ec2Client) ListLaunchTemplates(ctx context.Context) ([]*clients.LaunchTemplate, *string, error) {


Why pointer? My first idea is empty string = no token.

internal/clients/http/ec2/ec2_client.go

lzap · 2023-09-04T07:35:48Z

internal/clients/http/gcp/gcp_client.go

 	}

-	return templatesList, nil
+	return templatesList, &nextToken, nil


Ditto - the same concern.

lzap · 2023-09-04T07:39:29Z

internal/page/page.go

@@ -33,12 +34,12 @@ type Links struct {
 	Next     string `json:"next"`
 }
 type Metadata struct {
-	Total int `json:"total"`
+	Total int `json:"total,omitempty"`
 }

 type Info struct {


My suggestion would be renaming this to Metaroot instead of Info, that would include NewMetaroot.

Alternatively, I suggest to rename Metadata to just Data (keeping variable and JSON field as "metadata") and Info to Metadata.

Finally, my third suggestion is simply move links into metadata as this sets a bad practice - it feel like it is okay to pollute the root JSON namespace with fields while a single "metadata" field could be a nice solution with no other fields added.

lzap · 2023-09-04T07:41:15Z

internal/page/page.go

 func (o limitOffset) String() string {
 	return strconv.Itoa(int(o))
 }

-func APIInfoResponse(ctx context.Context, r *http.Request, total int) *Info {
+func NewInfo(ctx context.Context, r *http.Request, total int) *Info {


Maybe NewOffsetInfo and the other one NewTokenInfo? (Alternatively NewOffsetMetadata if you choose to do the refactoring I recommend.)

internal/routes/all_routes.go

lzap

This is so much cleaner, only documentation typos.

@ezr-ondrej are you happy with this?

lzap · 2023-09-05T14:00:22Z

cmd/spec/parameters.go

+
+var TokenQueryParam = Parameter{
+	Name:        "token",
+	Description: "The token used for requesting the next page of results; empty for the first page",


Suggested change

Description: "The token used for requesting the next page of results; empty for the first page",

Description: "The token used for requesting the next page of results; empty for the last page",

That was not what I meant here, I meant that an empty token leads to the first page.

Oh it is a bit unclear but it is fine, thanks.

internal/clients/interface.go

lzap · 2023-09-05T14:11:33Z

Tests are failing with iqe_provisioning_api.exceptions.ApiAttributeError: V1ListLaunchTemplateResponse has no attribute 'metadata' at ['received_data']['metadata'] and I guess there is a problem that "metadata" root field is missing after the refactoring.

There are two options, you can either create "Metaroot" type for the root field, or make use of Go anonymous struct directly in NewMetadata functions.

adiabramovitch · 2023-09-06T07:25:09Z

It is a problem in the iqe tests in my opinion. @akhil-jha ?

lzap · 2023-09-06T08:53:19Z

internal/payloads/launch_template_payload.go

-func (s *LaunchTemplateResponse) Render(_ http.ResponseWriter, _ *http.Request) error {
-	return nil
+	Data     []*LaunchTemplateResponse `json:"data" yaml:"data"`
+	Metadata page.Metadata             `json:"metadata" yaml:"metadata"`


Ah here it is, so the root is correct actually. Tests need to be updated I guess.

lzap · 2023-09-06T10:55:49Z

I think IQE REST client needs to be regenerated, let’s wait what @akhil-jha or @mshriver have to say.

lzap · 2023-09-07T09:12:35Z

@adiabramovitch you closed the PR?

adiabramovitch requested a review from lzap August 2, 2023 07:59

adiabramovitch marked this pull request as draft August 2, 2023 08:02

adiabramovitch force-pushed the token-sup branch from f418b03 to d5175a7 Compare August 2, 2023 12:44

lzap requested changes Aug 2, 2023

View reviewed changes

internal/payloads/launch_template_payload.go Outdated Show resolved Hide resolved

internal/clients/http/gcp/gcp_client.go Outdated Show resolved Hide resolved

adiabramovitch force-pushed the token-sup branch 3 times, most recently from 65e5373 to a0b1df6 Compare August 6, 2023 10:53

ezr-ondrej reviewed Aug 7, 2023

View reviewed changes

adiabramovitch force-pushed the token-sup branch from a0b1df6 to ce06412 Compare August 30, 2023 11:13

adiabramovitch commented Aug 30, 2023

View reviewed changes

adiabramovitch marked this pull request as ready for review August 30, 2023 14:29

adiabramovitch requested a review from a team as a code owner August 30, 2023 14:29

adiabramovitch requested review from ezr-ondrej and lzap September 3, 2023 07:20

lzap requested changes Sep 4, 2023

View reviewed changes

adiabramovitch force-pushed the token-sup branch 2 times, most recently from 78ee1aa to 1921d41 Compare September 4, 2023 10:58

adiabramovitch requested a review from lzap September 5, 2023 08:26

lzap requested changes Sep 5, 2023

View reviewed changes

lzap approved these changes Sep 6, 2023

View reviewed changes

adiabramovitch force-pushed the token-sup branch from 1921d41 to 6758309 Compare September 6, 2023 10:05

adiabramovitch closed this Sep 7, 2023

adiabramovitch force-pushed the token-sup branch from 6758309 to 8b1a4a5 Compare September 7, 2023 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(HMS-2248): Add token support for list templates #626

feat(HMS-2248): Add token support for list templates #626

adiabramovitch commented Aug 2, 2023 •

edited

Loading

lzap left a comment

akhil-jha commented Aug 3, 2023

adiabramovitch commented Aug 6, 2023 •

edited by ezr-ondrej

Loading

ezr-ondrej left a comment

lzap commented Aug 7, 2023

akhil-jha commented Aug 8, 2023

ezr-ondrej commented Aug 9, 2023

adiabramovitch Aug 30, 2023

lzap left a comment

lzap Sep 4, 2023

lzap Sep 4, 2023

lzap Sep 4, 2023

lzap Sep 4, 2023

lzap left a comment

lzap Sep 5, 2023

adiabramovitch Sep 6, 2023

lzap Sep 6, 2023

lzap commented Sep 5, 2023

adiabramovitch commented Sep 6, 2023

lzap Sep 6, 2023

lzap commented Sep 6, 2023

lzap commented Sep 7, 2023

	Description: "The token used for requesting the next page of results; empty for the first page",
	Description: "The token used for requesting the next page of results; empty for the last page",

feat(HMS-2248): Add token support for list templates #626

feat(HMS-2248): Add token support for list templates #626

Conversation

adiabramovitch commented Aug 2, 2023 • edited Loading

lzap left a comment

Choose a reason for hiding this comment

akhil-jha commented Aug 3, 2023

adiabramovitch commented Aug 6, 2023 • edited by ezr-ondrej Loading

ezr-ondrej left a comment

Choose a reason for hiding this comment

lzap commented Aug 7, 2023

akhil-jha commented Aug 8, 2023

ezr-ondrej commented Aug 9, 2023

Choose a reason for hiding this comment

lzap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Sep 5, 2023

adiabramovitch commented Sep 6, 2023

Choose a reason for hiding this comment

lzap commented Sep 6, 2023

lzap commented Sep 7, 2023

adiabramovitch commented Aug 2, 2023 •

edited

Loading

adiabramovitch commented Aug 6, 2023 •

edited by ezr-ondrej

Loading