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(gateway, cargo-shuttle): implement pagination for project list #862

Merged
merged 20 commits into from
May 31, 2023

Conversation

AlphaKeks
Copy link
Contributor

Please write a summary of your changes and why you made them.

Closes #845. This adds pagination functionality to the project list and deployment list subcommands for cargo-shuttle and changes the API endpoints to accommodate that.

How Has This Been Tested (if applicable)?

Unit tests in gateway/src/service.rs and deployer/src/persistence/mod.rs

AlphaKeks and others added 2 commits May 4, 2023 15:14
This change adds query parameters to the `/projects` endpoint that allow
specifying an `offset` and a `limit` to allow for pagination. The
`cargo-shuttle` CLI has also been updated to take those as optional
parameters for the `project list` subcommand with default values of `0`
for `offset` and `10` for `limit`.

refactor(deployer, cargo-shuttle): implement pagination for `deployment list`

This adds pagination functionality to `cargo shuttle deployment list`
just like `cargo shuttle project list` and allows the same parameters.
…ist`

This adds the same pagination functionality to `cargo shuttle deployment
list` as `cargo shuttle project list`.
@oddgrd oddgrd requested a review from iulianbarbu May 4, 2023 13:41
@oddgrd oddgrd added SB2 labels May 4, 2023
@Kazy
Copy link
Contributor

Kazy commented May 4, 2023

Regarding the number of results returned, by default the API will keep returning every single result, since we believe the endpoint is also used in the web console. And to go with it, there is also no limitation of the limit parameter as well.

The CLI though does pagination by default, returning the first 10 items, which is a breaking change. We wanted to also improve the experience on that on two fronts but didn't to first get your opinion on how we should solve them (maybe in an other PR). First there is no mention that more pages are available. Second in the case of a limit of 0 or of a page that goes beyond the actual number of pages; there is a mention that no deployment/project exists, which isn't strictly correct.

The easy solution is just to rework the messages. We can for instance say that there might be more results available in the next page, even if that isn't the case. The other solution would be to fetch the total number of items, using either a second SQL request, or a single one but with a count(*) over() to get the total count. A less expensive call could be done by just fetching if there is a new row or not (lead() over()).

@AlphaKeks
Copy link
Contributor Author

As I mentioned in our private conversation, we could very well make the --limit flag default to u32::MAX which would effectively keep the old behavior.

As for the potential issue with supplying a --page value that's too high, I would probably run 2 queries, returning early if the specified page is too high. But I'm open to do something else as well.

Copy link
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

Thanks @AlphaKeks ! The PR looks really good, it's almost there. Just left a few questions that are unclear on my side.

cargo-shuttle/src/args.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/args.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/client.rs Show resolved Hide resolved
cargo-shuttle/src/lib.rs Show resolved Hide resolved
deployer/src/handlers/mod.rs Show resolved Hide resolved
deployer/src/persistence/mod.rs Show resolved Hide resolved
gateway/src/api/latest.rs Outdated Show resolved Hide resolved
gateway/src/api/latest.rs Show resolved Hide resolved
gateway/src/service.rs Outdated Show resolved Hide resolved
gateway/src/service.rs Show resolved Hide resolved
fix(cargo-shuttle): remove long name in page param

fix(cargo-shuttle): reorder params in get_deployments path

fix(cargo-shuttle): check it limit is 0 in deployments_list to prevent useless code

fix(deployer, gateway): Move PaginationDetails struct to common

fix(gateway): Chain query builder params
AlphaKeks and others added 2 commits May 5, 2023 13:47
Reorders arguments to be consistent between `get_projects_list` and
`get_deployments`, and gets rid of an unused import.
@iulianbarbu
Copy link
Contributor

Hey @Kazy and @AlphaKeks ! How is everything going? Let me know if you need any help on this.

@iulianbarbu
Copy link
Contributor

Also FYI, this project was just changed to a medium one, because of the changed requirements. Let us know if you have any questions.

@iulianbarbu iulianbarbu added M and removed S labels May 16, 2023
@Kazy
Copy link
Contributor

Kazy commented May 18, 2023

Hey @iulianbarbu ! I've updated the branch with some of the changes we discussed.

Regarding the created_at column, I assumed everything was using Postgres, but this isn't the case when developing locally where instead Sqlite is used. I don't know if you actually support both at the same time (PSQL in prod, Sqlite in dev) or if you also use Sqlite in the context of the gateway and the deployer in production ?

In the meantime I've made the changes to use project_name for ordering in the gateway and last_updated_at (latest updated first, as it makes more sense when using the CLI) for the deployer. I've also added a text that an another page might be available: happy to rewrite it in any way.

@iulianbarbu
Copy link
Contributor

Hey @iulianbarbu ! I've updated the branch with some of the changes we discussed.

Regarding the created_at column, I assumed everything was using Postgres, but this isn't the case when developing locally where instead Sqlite is used. I don't know if you actually support both at the same time (PSQL in prod, Sqlite in dev) or if you also use Sqlite in the context of the gateway and the deployer in production ?

We use sqlite in prod as well. We use it for both gateway and deployer at this moment.

In the meantime I've made the changes to use project_name for ordering in the gateway and last_updated_at (latest updated first, as it makes more sense when using the CLI) for the deployer. I've also added a text that an another page might be available: happy to rewrite it in any way.

Thanks, taking a look rn.

Copy link
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

Looks good so far @Kazy ! I think the only thing left is to add a migration for the projects table, where we alter the table by adding the created_at column. This column should be changed when creating a new project, info persisted in the gateway sqlite. And then, when iterating through the account projects, we should ORDER BY created_at DESC, project_name. What do you think?

@Kazy
Copy link
Contributor

Kazy commented May 19, 2023

Hey @iulianbarbu ! Thank you for the review !

I've added the created_at column to the gateway DB. I didn't use a DEFAULT statement when updating the column because with SQLite you cannot add a non constant default value for a column (in our case CURRENT_TIMESTAMP).

@Kazy
Copy link
Contributor

Kazy commented May 25, 2023

Hey @iulianbarbu, I've fixed the tests, everything should be good for this PR now. Let me know if there is anything left to do :)

Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

This looks really great, thank you for all your work on this @Kazy and @AlphaKeks! 😍

@oddgrd oddgrd merged commit b7e5e3b into shuttle-hq:main May 31, 2023
@Kazy Kazy deleted the feat/cli-pagination branch May 31, 2023 15:33
iulianbarbu added a commit that referenced this pull request Jun 6, 2023
* cargo-shuttle: separated unix from windows local_run (#823)

* docs: add note about init bug to readme (#824)

Add a note about #821 to the readme.

* ci: fix windows binary build (#825)

* ci: test prod ci

* ci: restore ci

* chore: v0.15.0 (#820)

* chore: v0.15.0

* chore: bump examples submodule

* revert: protoc removal (#826)

* revert: protoc removal

* ci: test run release jobs

* revert: dockerfile protoc install

* revert: deployer prepare.sh protoc install

* ci: restore ci

* fix: wasm qa casing (#828)

* fix: disable docker QA (#830)

* fix: disable docker QA

* ci: keep running QA

* fix: Remove unused project list filtering (#832)

* fix: Project filtering is disabled

* remove filter flag

---------

Signed-off-by: jonaro00 <54029719+jonaro00@users.noreply.github.com>

* add star gif

* docs: document how to generate protofiles (#836)

* docs: document how to generate protofiles

* docs: expand on crate description

* feat: refactor deployer to run locally without auth (#810)

* feat: refactor deployer to run locally without auth

* docs: update contributing deployer guide

* refactor: workspace dep

* fix: bump regex to 1.8.1 to fix 1.8.0 bug

* fix: provisioner port for local deployer

* refactor: renaming

* feat: refactor to request token from auth

* refactor: remove claims feature from deployer common

* refactor: use key/cookie from original request

* refactor: implement scopebuilder

* fix: clippy

* refactor: address review

* refactor: auth container command

* refactor: cleanup builder in start fn

* docs: local arg docs

* added shuttle console sneak peek

* typo-fix

* fix: shuttle init --template, reorder subcommands, fix bugs (#792)

* shuttle init --template, reorder subcommands, fix bugs

* fix review feedback

* ci: download sccache instead of compiling it (#859)

* ci: download sccache instead of compiling it

* ci: fix some typos

* chore: more typos

* build(docker): Change default provisioner port to 3000 (#852)

On MacOs, port 5000 is used by AirPlay receiver which will make the 'make up' command described in CONTRIBUTING.md fail for Mac users

* feat: ApiKey newtype to ensure key is always valid format (#835)

* feat: ensure API key is valid

* feat: use ApiKey in auth

* refactor: clean up tests

* refactor: don't allocate in parse unless it succeeds

* fix: clippy

* fix: missing anyhow

* feat: impl debug/display for apikey

* chore: add `.editorconfig` (#855)

This adds a simple `.editorconfig` file to the root of the repository so
that different editors/IDEs may pick up the settings. This makes it
easier to have consistent formatting while editing, not just after
running `cargo fmt`. See [here](https://editorconfig.org) for more details.

Co-authored-by: AlphaKeks <alphakeks@dawn.sh>

* fix: `make test` (#858)

* fix: `make test`

* fix: update api-key

* fix: some panic messages get lost (#854)

* fix: some panic messages get lost

* feat: tests for loader and bind panics

---------

Co-authored-by: Thomas Grimm <thomas@boros.world>

* feat: remove /hello from tests/ci (#863)

* feat: change "/hello" routes to "/"

* feat: use updated examples

* fix: clippy complaining about format!{"{url}") being useless post '/hello' removal

* chore: run cargo fmt

* chore: update examples

---------

Co-authored-by: Paul Otten <potten@my.bcit.ca>
Co-authored-by: Paul Otten <lightnica@yahoo.ca>

* feat: add on_new_span impl to runtime Logger (#864)

* feat: add on_new_span impl to runtime Logger

* PR suggestion

Co-authored-by: Pieter <pieter@chesedo.me>

---------

Co-authored-by: Vlad Stepanov <8uk.8ak@gmail.com>
Co-authored-by: Pieter <pieter@chesedo.me>

* misc: rename examples to shuttle-examples (#871)

* misc: rename examples to shuttle-examples

* misc: update examples repo

* chore: v0.16.0 (#881)

* chore: v0.16.0

* chore: bump examples

* feat(Makefile): add option to disable --detach on make up (#878)

* feat(Makefile): add option to disable --detach on make up

* fix: minor formatting changes

* fix: revert addition of apikey to auth (#886)

* fix: revert addition of apikey to auth

* fix: display impl is needed for key.to_string()

* revert: revert #886 (#887)

* Revert "fix: revert addition of apikey to auth (#886)"

This reverts commit 7054e6a.

* feat: add debug call for malformed api key

* add option to use rustls instead of native-tls in `shuttle-shared-db` (#870)

* add option to use rustls instead of native-tls in `shuttle-shared-db`

* Update CircleCI config

Allow specifying features by one for a specific crates.
Test `shuttle-shared-db` features one by one.

* more readable CI job name

* Add comment to CircleCI config

* Match doc links with Shuttle Service current doc url (#885)

* match doc links with current url

* missing test error comparisons

* Update/syn 2.0 (#880)

* update syn to 2.0

* feat: codegen upgraded to use syn v2 (close #875)

---
Co-Authored-by: John Vandenberg <jayvdb@gmail.com>
Co-Authored-by: Yatin Maan

* fix: appropriate expect statement (#875)

* chore: update aws crates (#897)

* chore: update aws crates

* fix: remove accidental comment

* fix:  set correct admin scopes in scopebuilder (#899)

* chore: bump common to 0.16.2 (#900)

* chore: bump common to 0.16.1

* chore: v0.16.2

* Update README.md

updated capital

* Update README.md

updated capitalization of Shuttle

* Reimplemented JwtAuthentication with struct-based Future. (#868)

* Reimplemented JwtAuthentication with struct-based Future.

* More effective encoding

* Remove explicit lifetime

* Code cleanup

* Code cleanup

---------

Co-authored-by: root <root@razor.localdomain>

* docs: add installation instructions for Arch Linux (#902)

* feat: show output of failed tests (#907)

* feat: show output of failed tests

* feat: remove cargo dependency

* fix: cargo clippy warnings

* fix: Cargo.lock version

* ci: release automation on unstable (#816)

* ci: push images to unstable on main with approval

* ci: rename approve unstable push job

* ci: test unstable ssh access

* ci: test master access

* ci: test ssh access with generated config

* ci: try StrictHostKeyChecking no

* ci: try circleci user

* ci: try host admin.unstable

* ci: try unstable deployment

* ci: only deploy on push success

* ci: restore other jobs

* ci: remove sparse registry from deploy

* ci: remove test

* chore: upgrade salvo in shuttle-salvo (#901)

* chore: upgrade salvo in shuttle-salvo

* docs: hello_world at root

* feat: allow resetting a user's API-key (#857)

* feat: allow resetting a users API-key

A user should be able to reset their api-key, this is important functionality to have in case users leak their key. This should generate a new api-key and persist it in the users table.

Fixes #838

* clippy

* chore: some cleanup, code style, etc.

* account name from cookie or key

* invalidate auth_layer cache

* fixes

* Add `logout --invalidate-api-key`

* Rename to reset, add success msg and login url hint.

* fmt

---------

Co-authored-by: jonaro00 <54029719+jonaro00@users.noreply.github.com>

* feat(cargo-shuttle): log reconnects and improved error messages (#853)

* feat(cargo-shuttle): log reconnects and improved error messages

* feat(cargo-shuttle): outputs about the reconnect

* chore: cargo fmt

* chore: cargo fmt

* made last_state optional, added more verbose state outputs, removed Clone trait struct Client and DeployArgs.

* refactor: refactoring cargo-shuttle library

add optional to last_state, change state output messages to have more information, and removed unused clone trait from stucts

* fix: typo in State: Unkown

Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>

* refactor: remove unused code

remove the running state arm as this state should be unreachable and remove Eq and PartialEq since last_state is optional now

---------

Co-authored-by: Forrest Walker <forrestpatwalker@hotmail.com>
Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>

* feat(runtime): Remove dependency on clap (#822)

* docs: contributing updates (#918)

* fix: deployment state shown as running on startup crash (#919)

* fix: deployment state shown as running on startup crash

* fix: remove second deployment fetch

* chore: promote hyper-reverse-proxy to a workspace dependency (#921)

this fixes `cargo vendor`

* suggest next logical command (#915)

* suggest next command

* suggest next command

* suggest next command

* fix: suggest next command

* fix: suggest new command

* fix: suggest next command

* fix: suggest next command

* fix: suggest next command

* ci: add windows qa (#812)

* ci: add windows qa

* ci: why???

* ci: windows qa

* ci: stop on error

* ci: test wasm

* ci: test docker

* ci: turn off docker for now

* ci: add --template flag

* ci: remove /hello from test endpoints

* ci: figure out sleep time

* ci: fix timeout

* ci: disable wasm

* ci: restore old

* fix(gateway): handle certificate expiration as well (#932)

* fix(gateway): handle certificate expiration as well

* gateway: commented the renewal check

* ci: production deployment automation (#920)

* ci: production deployment automation initial commit

* ci: incorrect requirement

* ci: fix production ssh config

* ci: test deduplicated deploy-images workflow

* ci: test deduplicated deploy workflow for prod

* ci: test deploying to unstable with envs in params

* ci: try using $ for env vars in params

* ci: fix password types

* release(prod): test dry run

* release(prod): enable unstable and prod image build/push...

...and deploy

* release(prod): test cargo publish dry run

* release(prod): cargo-shuttle publish crates conflicts with platform-test

* release(prod): improved with speed ups

---------

Co-authored-by: Iulian Barbu <iulianbarbu2@gmail.com>

* Chore/0.17.0 (#934)

* chore: release 0.17.0

* chore: updated deps versions

* examples: updated to latest shuttle-examples/main

* release(prod): gate against local crates.io patch (#936)

* release(prod): fix the missing line break escape (#937)

* release(prod): add protoc dependency and fix the crates order (#938)

* feat(shuttle-axum) Make AxumService generic to be able to use axum::State with it (#924)

* docs: Update links and commands (#948)

* update docs etc

* remove comment

* Update GitHub templates (#945)

* chore: update Cargo.lock (#942)

* Add helpful error if port cannot be used (#950)

* Add helpful error if port cannot be used

* Don't println on success

* cargo format

* fix: --name was ignored when not running from cargo folder (#929)

* fix: --name was ignored when not running from cargo folder

* fix: remove custom error message to fix tests

* Use `unwrap_or` instead of explicit match statement

Co-authored-by: jonaro00 <54029719+jonaro00@users.noreply.github.com>

* Fix cargo fmt

---------

Co-authored-by: jonaro00 <54029719+jonaro00@users.noreply.github.com>

* chore: bump otel crates and remove protoc dep (#956)

* fix: log files packed in archive (#931)

* fix: log files packed in archive

* fix: change log level to debug

* refactor: sanitize all path on the user's proxy (#946)

* feat(gateway, cargo-shuttle): implement pagination for project list (#862)

* feat(gateway, cargo-shuttle): implement pagination for `project list`

This change adds query parameters to the `/projects` endpoint that allow
specifying an `offset` and a `limit` to allow for pagination. The
`cargo-shuttle` CLI has also been updated to take those as optional
parameters for the `project list` subcommand with default values of `0`
for `offset` and `10` for `limit`.

refactor(deployer, cargo-shuttle): implement pagination for `deployment list`

This adds pagination functionality to `cargo shuttle deployment list`
just like `cargo shuttle project list` and allows the same parameters.

* feat(deployer, cargo-shuttle): implement pagination for `deployment list`

This adds the same pagination functionality to `cargo shuttle deployment
list` as `cargo shuttle project list`.

* Updated from PR suggestions

fix(cargo-shuttle): remove long name in page param

fix(cargo-shuttle): reorder params in get_deployments path

fix(cargo-shuttle): check it limit is 0 in deployments_list to prevent useless code

fix(deployer, gateway): Move PaginationDetails struct to common

fix(gateway): Chain query builder params

* fix(cargo-shuttle, deployer): get rid of warnings

Reorders arguments to be consistent between `get_projects_list` and
`get_deployments`, and gets rid of an unused import.

* feat(deployer): add missing pagination params to get_deployments utoipa

* test: add test for out of bound pagination of deploy and projects list

* feat(gateway): add order by clause for paginated endpoint

* ref: revert having PaginationDetails in common

* feat(cargo): improve error message when page == 1 vs page > 1

* feat(common): add message that more page might be available for projects

* feat(cargo, deployment): update pagination message for deployment list

* style(deployer): reformat perstistence/mod.rs file

* feat(deployer): return the deployments starting from the latest updated

* feat(cargo-shuttle): update next page message

* feat(gateway): add created_at field on projects, use it to sort projects

* style(cargo-shuttle): reformat files

* test(deployer): fix test with new get_deployments ordering

---------

Co-authored-by: AlphaKeks <alphakeks@dawn.sh>
Co-authored-by: Jocelyn Boullier <jocelyn@boullier.bzh>
Co-authored-by: 73nko <apramos89@gmail.com>

* fix: crossterm/comfytable conflict (#959)

* fix: crossterm/comfytable conflict

* ci: remove unused protoc installs

* feat: pre-installed build environment in deployer (#960)

* feat: pre-installed build environment in deployer

* Edit list

* refactor: un-tangle crossterm/comfytable (#961)

* refactor: un-tangle crossterm/comfytable

* chore: bump crossterm

* docs: comment typo

* fix: Ignore span logs below WARN (#958)

* fix: Ignore span logs below WARN

* modify test

* fmt

* move to before JsonVisitor, fix test

* Convert later

* fmt

* Flip the comparison like a burger

* fix: remove cargo-sort from CONTRIBUTING.md (#966)

* fix/release(prod): unstable AWS creds clashed with prod (#970)

* chore: v0.18.0 (#972)

* chore: v0.18.0

* chore: bump examples

* fix: broken cargo.lock after merge

---------

Signed-off-by: jonaro00 <54029719+jonaro00@users.noreply.github.com>
Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
Co-authored-by: jonaro00 <54029719+jonaro00@users.noreply.github.com>
Co-authored-by: Ivan <cernja@pm.me>
Co-authored-by: s e <iamawacko@protonmail.com>
Co-authored-by: Zisulin Morbrot <22527555+morlinbrot@users.noreply.github.com>
Co-authored-by: AlphaKeks <85143381+AlphaKeks@users.noreply.github.com>
Co-authored-by: AlphaKeks <alphakeks@dawn.sh>
Co-authored-by: paulotten <paulotten@users.noreply.github.com>
Co-authored-by: piewhat <hhwzmw5ql@relay.firefox.com>
Co-authored-by: Thomas Grimm <thomas@boros.world>
Co-authored-by: mikegin <gindin.mike@gmail.com>
Co-authored-by: Paul Otten <potten@my.bcit.ca>
Co-authored-by: Paul Otten <lightnica@yahoo.ca>
Co-authored-by: Valentin <59ichiigo@gmail.com>
Co-authored-by: Vlad Stepanov <8uk.8ak@gmail.com>
Co-authored-by: Pieter <pieter@chesedo.me>
Co-authored-by: Xavi <30369208+XaviFP@users.noreply.github.com>
Co-authored-by: Syed Fasiuddin <66054777+SyedFasiuddin@users.noreply.github.com>
Co-authored-by: Artūras Šlajus <x11@arturaz.net>
Co-authored-by: root <root@razor.localdomain>
Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>
Co-authored-by: piewhat <Piewhat@protonmail.com>
Co-authored-by: Heiko Seeberger <hseeberger@users.noreply.github.com>
Co-authored-by: Forrest Walker <forrestpatwalker@hotmail.com>
Co-authored-by: Kieren Davies <kieren@kdavi.es>
Co-authored-by: figsoda <figsoda@pm.me>
Co-authored-by: Iulian Barbu <iulianbarbu2@gmail.com>
Co-authored-by: Boyd Kane <33420535+beyarkay@users.noreply.github.com>
Co-authored-by: Raminder Singh <romi_ssk@yahoo.co.in>
Co-authored-by: Jocelyn Boullier <jocelyn@boullier.bzh>
Co-authored-by: 73nko <apramos89@gmail.com>
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.

[Feature]: Refactor GET-many endpoints in deployer to allow pagination
5 participants