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(ui): add more detail + filter to project list page #2201

Merged
merged 8 commits into from
Jul 2, 2024

Conversation

rbreeze
Copy link
Contributor

@rbreeze rbreeze commented Jun 24, 2024

Fixes #1836

Includes new API endpoint that provides a list of Stages for each Project.

Screenshot 2024-07-01 at 18 21 56 Screenshot 2024-06-24 at 15 48 13

Signed-off-by: Remington Breeze <remington@breeze.software>
@rbreeze rbreeze requested a review from a team as a code owner June 24, 2024 22:49
Copy link

netlify bot commented Jun 24, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit 5ec8866
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/66841ecd8f084d00086d3751
😎 Deploy Preview https://deploy-preview-2201.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rbreeze rbreeze requested review from krancour and hiddeco June 24, 2024 22:51
Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 45.83333% with 13 lines in your changes missing coverage. Please review.

Project coverage is 46.29%. Comparing base (2e6342a) to head (5ec8866).
Report is 4 commits behind head on main.

Files Patch % Lines
internal/api/list_projects_v1alpha1.go 52.38% 7 Missing and 3 partials ⚠️
internal/api/list_stages_v1alpha1.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2201      +/-   ##
==========================================
- Coverage   46.30%   46.29%   -0.01%     
==========================================
  Files         242      242              
  Lines       16775    16797      +22     
==========================================
+ Hits         7767     7776       +9     
- Misses       8636     8645       +9     
- Partials      372      376       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rbreeze added 2 commits June 26, 2024 10:14
Signed-off-by: Remington Breeze <remington@breeze.software>
Signed-off-by: Remington Breeze <remington@breeze.software>
@krancour
Copy link
Member

krancour commented Jun 28, 2024

I'm a little bit worried about the size of the responses from the new endpoint. It's well within the realm of possibility that the response will contain thousands of Stages -- and Stages can be rather large, given the extent of their Status subresources.

I'd feel better about it if we introduced paging to both the new endpoint and the Projects page -- sorry I know that's a fair bit of scope creep.

If we were to do that, I think it would be tenable to make pageSize list Stages requests. (One for each Project on the current page -- something I wouldn't have considered with an unbounded number of Project per page.)

Ultimately, that would remove the need for the new endpoint. The extent of the back-end changes would just be the addition of paging to the existing list Project endpoint.

Tangentially, I think there may be some security considerations to weigh here as well. Regardless of whether we were to make the suggested changes above or not, there may very likely be an issue of users who are able to list all Projects being unable to list Stages, which makes all of this a tad trickier.

If we were to swap to the paging approach and using existing endpoints, I think we'd just have to make the UI silently ignore 403s from list Stages requests for any given project.

@rbreeze
Copy link
Contributor Author

rbreeze commented Jul 2, 2024

@krancour Agreed! Updated this PR accordingly, and added simple string filtering in the process

@rbreeze rbreeze changed the title feat(ui): add more information to project list page feat(ui): add more detail + filter to project list page Jul 2, 2024
Signed-off-by: Remington Breeze <remington@breeze.software>
Comment on lines +30 to +32
ResourceType string `json:"resourceType,omitempty" protobuf:"bytes,1,opt,name=resourceType"`
ResourceName string `json:"resourceName,omitempty" protobuf:"bytes,2,opt,name=resourceName"`
Verbs []string `json:"verbs,omitempty" protobuf:"bytes,3,rep,name=verbs"`
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to figure out why these struct tags were absent before and have now shown up all of the sudden. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This is non-blocking, btw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering the same thing. Maybe a library used in the tooling was updated recently, but this is the first PR since which had any codegen?

Comment on lines +335 to +336
optional int32 page_size = 1;
optional int32 page = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for thinking to make these optional! It preventing breaking the CLI!

@krancour
Copy link
Member

krancour commented Jul 2, 2024

@rbreeze took this for a test drive and works very nicely.

Excellent work on this.

I did notice that the list Stages endpoint isn't returning Stages lexically ordered by name. That's not something new you introduced, but the updated Projects page is making that discrepancy more noticeable. (Surprised no one noticed this when using the CLI.)

Screenshot 2024-07-01 at 11 21 25 PM

Would you mind adding a sort here?

stages := make([]*kargoapi.Stage, len(list.Items))
for idx := range list.Items {
stages[idx] = &list.Items[idx]
}
return connect.NewResponse(&svcv1alpha1.ListStagesResponse{
Stages: stages,
}), nil

Signed-off-by: Remington Breeze <remington@breeze.software>
@rbreeze
Copy link
Contributor Author

rbreeze commented Jul 2, 2024

@krancour Good idea! Updated ListStage endpoint with a sort.

@rbreeze rbreeze requested a review from krancour July 2, 2024 15:17
Comment on lines 33 to 35
sort.Slice(list.Items, func(i, j int) bool {
return list.Items[i].Name < list.Items[j].Name
})
Copy link
Member

@krancour krancour Jul 2, 2024

Choose a reason for hiding this comment

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

Since Go 1.21, this is the preferred way to sort:

Suggested change
sort.Slice(list.Items, func(i, j int) bool {
return list.Items[i].Name < list.Items[j].Name
})
slices.SortFunc(list.Items, func(a, b kargoapi.Stage) int {
return strings.Compare(a.Name, b.Name)
})

Note, we do have a lot of places in the code base where it's still done the older way and I'll open a separate chore issue for fixing those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks for catching that! Updated the ListProject sort too. Sorry for the back and forth

Signed-off-by: Remington Breeze <remington@breeze.software>
@rbreeze rbreeze requested a review from krancour July 2, 2024 15:38
@rbreeze rbreeze added this pull request to the merge queue Jul 2, 2024
Merged via the queue into main with commit a5887d9 Jul 2, 2024
16 of 17 checks passed
@rbreeze rbreeze deleted the project-list-details branch July 2, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: simple filter/search bar on projects page
3 participants