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

Add section on pagination design to README #128

Merged
merged 4 commits into from
Sep 29, 2023

Conversation

cmurphy
Copy link
Contributor

@cmurphy cmurphy commented Sep 20, 2023

No description provided.

Remove unnecessary items from the `access` slice in one of the unit
tests. Each item in `access` corresponds to one request in `apiOps`, so
they should have the same number of elements.
Add a section describing the design of pagination-related query
parameters (filter, sort, page, etc) to the README, including how
testing is implemented.
@cmurphy cmurphy requested review from a team and MbolotSuse September 20, 2023 21:03
@cmurphy cmurphy requested a review from KevinJoiner September 27, 2023 18:07
@cmurphy
Copy link
Contributor Author

cmurphy commented Sep 27, 2023

Fixed a typo and added some missing information to the API section

Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

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

One nit, but otherwise LGTM, thanks for adding docs.


![](./docs/store-flow.svg)

#### Unit tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: While I think that the section on integration tests is useful (since those are in a separate repo in a hard-to-identify location), I'm not sure that I feel the same way on the unit tests. As long as you know what packages the code is in, it's easy to identify the test files. I would recommend just identifying the packages that are relevant to the list processor instead of including the section on unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be obvious where the unit tests are, but I think it's less obvious what to put in those unit tests. In some cases making changes to pkg/stores/partition/listprocessor could have an effect on the output of pkg/stores/partition/store.go, and it's worthwhile to add tests in both places even if the code in store.go didn't change (even if that's stretching the definition of "unit"), so I think it's valuable to keep that recommendation here.

@cmurphy cmurphy requested review from a team, tomleb, JonCrowther and maxsokolovsky September 28, 2023 19:54
@cmurphy cmurphy merged commit 811e0b3 into rancher:master Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants