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 releases and flux runtime pages #1451

Merged
merged 3 commits into from Feb 17, 2022
Merged

Add releases and flux runtime pages #1451

merged 3 commits into from Feb 17, 2022

Conversation

jpellizzari
Copy link
Contributor

@jpellizzari jpellizzari commented Feb 15, 2022

Adds the list pages according to the provided designs https://www.figma.com/file/IVHnM9iyeFWpd11evtY8ux/Weave-GitOps?node-id=5399%3A12874

Releases/Applications:
Screenshot from 2022-02-15 15-15-11

Sources Page:
Screenshot from 2022-02-15 15-08-23

Flux Runtime:
Screenshot from 2022-02-15 15-13-46

Note that this branch will not work for local development because of other broken things on v2. See this patch for more details: https://gist.github.com/jpellizzari/115de69875911c37cb6b0c0edc38e20c

@@ -119,7 +119,7 @@ service Apps {
*/
rpc ListKustomizations(ListKustomizationsReq) returns (ListKustomizationsRes) {
option (google.api.http) = {
get : "/v1/namespace/{namespace}/app/{app_name}/kustomization"
get : "/v1/kustomizations"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should this be plural? Usually the REST semantics are /v1/kustomization, I would expect a list. Then /v1/kustomization/:id, I would expect to get one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but not sure why we would try to min/max this as a REST API. I would hope someone would use the open API output to generate a client library in their language of choice.

We already gen the go and ts clients. I didn't have to change anything in my "user space" code to accommodate this change.

@sympatheticmoose
Copy link
Contributor

Good progress being made 👍 thanks Jordan

Some comments:
Applications page

  • Types should be kustomization / Helm Release rather than kustomize
  • Source in the designs doesn't show the type, but actually I'm ok with it cc @bia
  • Missing filters (assuming follow up PR)
  • Name is only one with sort arrow, is this easy to add to other columns?

Sources page

  • should be sub-menu item under Applications similar to Clusters > templates in WGE.
  • Types should be Git Repository / Helm Repository / Bucket (i.e. not just Git)
  • Missing Reference (currently marked as Release in Figma)
  • Designs are yet to address comments/reflect the doc in notion and as such are missing cluster, namespace and interval/last synced

Flux Runtime page

  • Image not Images
  • Default should sort by Cluster.

@jpellizzari
Copy link
Contributor Author

jpellizzari commented Feb 16, 2022

@sympatheticmoose

  • Types should be kustomization / Helm Release rather than kustomize

Fixed

  • Missing filters (assuming follow up PR)

@joshri is working on that

  • Name is only one with sort arrow, is this easy to add to other columns?

Fixed

Sources page

  • should be sub-menu item under Applications similar to Clusters > templates in WGE.

Follow up created: #1459

  • Types should be Git Repository / Helm Repository / Bucket (i.e. not just Git)

Fixed

  • Missing Reference (currently marked as Release in Figma)

Added, but we need to think about what this means for Helm/Bucket

  • Designs are yet to address comments/reflect the doc in notion and as such are missing cluster, namespace and interval/last synced

Interval added

Flux Runtime page

  • Image not Images

Fixed, but a Deployment can have multiple images.

  • Default should sort by Cluster.

Added

@jpellizzari jpellizzari force-pushed the jp-v2-releases-page branch 2 times, most recently from 56aa620 to 6e2a446 Compare February 16, 2022 19:06
@jpellizzari jpellizzari marked this pull request as ready for review February 16, 2022 19:07
@jpellizzari jpellizzari force-pushed the jp-v2-releases-page branch 3 times, most recently from d4e11c3 to c3c8d36 Compare February 16, 2022 21:07
@sympatheticmoose
Copy link
Contributor

Many thanks @jpellizzari

Added, but we need to think about what this means for Helm/Bucket

Both still have status.artifact.revision or do you feel this isn't valuable/there's something else we should look to show?

Fixed, but a Deployment can have multiple images.

Fair point, I wouldn't expect the flux controllers to though? Or am I missing something?


list := &appsv1.DeploymentList{}

if err := k8s.List(ctx, list, client.InNamespace(msg.Namespace)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid making assumptions about flux namespace (https://github.com/weaveworks/weave-gitops/pull/1451/files#diff-35acf2adc3a7b1021fb41d3473b47e030d3708ba2e8ff7a4b76c13ee80b4076cR42) and grab it by using something like: https://github.com/weaveworks/weave-gitops/blob/v2/pkg/kube/kubehttp.go#L496

I'm not sure we want to keep that interface but the actual method is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luizbafilho Since we allow the client to specify the namespace, can I fetch the correct namespace in a different call (in a future PR)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it would be a good follow up issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@joshri joshri left a comment

Choose a reason for hiding this comment

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

Front end looks stunning but I'll leave the approval to the back end experts

@sympatheticmoose are all sortable fields intended to have a sort arrow? Right now only the field that is actively being sorted has one


export default styled(KubeStatusIndicator).attrs({
className: KubeStatusIndicator.name,
})``;
Copy link
Contributor

Choose a reason for hiding this comment

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

💕 ❤️ 💕

Copy link
Contributor

Choose a reason for hiding this comment

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

@sympatheticmoose are all sortable fields intended to have a sort arrow? Right now only the field that is actively being sorted has one

I believe that is the design... @bia could you confirm? I am assuming the column header is clickable and thats what applies/flips the sort?

Copy link

Choose a reason for hiding this comment

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

Yes, in the current design only the field that is actively being sorted has an arrow (facing up or down based on sort direction). The other column headers that can sort the table should be clickable and the sort would be applied on click.

"type",
"cluster",
"status",
"last synced at",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if my sort is handling "last synced at" correctly...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probs not; we will need to add the concept of data types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshri joshri self-requested a review February 17, 2022 16:14
@jpellizzari jpellizzari merged commit c8c743f into v2 Feb 17, 2022
@jpellizzari jpellizzari deleted the jp-v2-releases-page branch February 17, 2022 16:29
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.

None yet

6 participants