-
Notifications
You must be signed in to change notification settings - Fork 259
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: Allow filtering projects by project status #613
Conversation
* add logout function
* Adds a flag for dynamically filtering projects by deployment status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Thanks josh. I added some comments about how I think we could make this a bit more efficient. I'd also like to point out that this is filtering by project status, not deployment status, but I realize it is a bit confusing 😄 The project status indicates the status of the deployer container, while the deployment status indicates the status of the deployed service within. Being able to filter the results from deployments list
is also something we want to support though, and/or pagination of the results. But that would be a separate issue/PR.
I'm also wondering if we should be able to filter by more than those three states, but I'll have to get back to you on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This looks good to go, but would you mind adding a couple of assertions next to the iter_user_projects_detailed
test? See https://github.com/shuttle-hq/shuttle/blob/main/gateway/src/service.rs#L709-L716
No worries - I'm at work at the moment but this will be done by end of tomorrow or over the weekend if not tomorrow. |
Accidentally pushed too early - please ignore 🤦 |
Should be ready to review now, some relevant assertions were added. Let me know if anything needs changing before merging |
LGTM, thanks! 🥳 |
Ref: Issue #612
Notes:
Please ignore logout function removal - this is due to incorrect branching from an earlier PR unfortunately, some caution may not have been exercised 😅