Skip to content
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

feat: add support for GetProjectBatch and GetVersionBatch #73

Merged
merged 4 commits into from
May 28, 2024

Conversation

zaibon
Copy link
Contributor

@zaibon zaibon commented May 24, 2024

The API for both endpoint is paginated, so this implementation return an iterator that allow the library user to controle how he wants to consume the response.

The API for both endpoint is paginated, so this implementation return an iterator that allow the library user to controle how he wants to consume the response.
@auto-assign auto-assign bot requested a review from edoardottt May 24, 2024 09:29
@edoardottt
Copy link
Owner

Hi @zaibon ! thanks so much for the PR.
did u try to fix the linting errors? if not, it's not a problem don't worry. I'll check those locally once the PR is merged

NextPageToken string `json:"nextPageToken"`
}

func getProjectBatch(ctx context.Context, c *client.Client, projectNames []string) <-chan batchJob[def.Project] {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 2 methodsgetProjectBatch and getVersionBatch are very similar. I wonder if there is a way to refactor them into a single function...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a way to extract the duplicate logic.
If you found this becoming too much magic, I can drop the last commit f8e0bd0

@zaibon
Copy link
Contributor Author

zaibon commented May 24, 2024

@edoardottt I addressed the linting errors. CI is happy now 😄

@edoardottt edoardottt self-assigned this May 25, 2024
@edoardottt
Copy link
Owner

edoardottt commented May 25, 2024

Thank u so much @zaibon. I'm gonna review the PR.
Before that, which tests did you use? So I can try them (plus others if necessary) locally. Specifically I have in mind edge cases, e.g. client requests the Nth page at first instead of the actual first page of the paginated response (could exist, could be missing), etc..

Thanks again :)

@zaibon
Copy link
Contributor Author

zaibon commented May 25, 2024

At the moment, my implantation does not support requesting a specific page. It only exposes an easy API to walk over all the pages in order. The API itself does not really make it easy to request a specific page because it does not use a number of pages in the request. It uses a token based approach. So you only know the next token after seeing the first page.

@edoardottt edoardottt added enhancement New feature or request golang labels May 26, 2024
Copy link
Owner

@edoardottt edoardottt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!
Left some thougths, waiting to read what you think about :)


func TestGetProjectBatch(t *testing.T) {
t.Run("GetProject batch", func(t *testing.T) {
iter, err := api.GetProjectBatch([]string{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This input returns a response with 3 items in a single page. Can we have a test checking for correct results in paginated results too?
We don't have to check the responses' content, just if the items count match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test that returns 300 results.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

results, err := consumeIter(iter)
require.NoError(t, err)

assert.Equal(t, expected, results)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like previously in other discussions, I don't know if this is the best way to check the correctness of code.
E.g. I could release a new version of defangjs and this test case will fail :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it will not since we're requesting details of a specific version. The logic is the same used in the TestGetVersion test.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your're right

Copy link
Owner

@edoardottt edoardottt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@edoardottt edoardottt merged commit 3405360 into edoardottt:devel May 28, 2024
5 checks passed
@edoardottt edoardottt linked an issue May 28, 2024 that may be closed by this pull request
@edoardottt
Copy link
Owner

Thanks so much @zaibon !
Your contribution is really appreciated :)

@zaibon zaibon deleted the v3Alpha-batch branch May 28, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request golang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Batch
2 participants