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

Workloads status component for resource list components #3637

Closed
wants to merge 2 commits into from

Conversation

ajatprabha
Copy link
Contributor

@ajatprabha ajatprabha commented Mar 8, 2019

This PR adds workloads status section charts to the resource list components.
Works towards #3440

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 8, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ajatprabha
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: pewu

If they are not already assigned, you can assign the PR to them by writing /assign @pewu in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from jeefy and PeWu March 8, 2019 01:08
@jeefy
Copy link
Member

jeefy commented Mar 8, 2019

Have you rebased to master recently? There are some files/changes in there I know have been removed as of the Angular migration in this PR. Totally get it's a WIP, just looking to help out. 😄

Thanks!

@ajatprabha
Copy link
Contributor Author

Yes, I rebased with master before creating the PR. The changes are working fine locally. Can you point out the files that need attention, I'll make the necessary changes after I add the statuses for workloads.

@maciaszczykm
Copy link
Member

maciaszczykm commented Mar 8, 2019

Pointed a few places, please review all of your changes or cherry pick your commits into the new branch created from current master.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 8, 2019
@codecov
Copy link

codecov bot commented Mar 9, 2019

Codecov Report

Merging #3637 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3637      +/-   ##
==========================================
- Coverage   46.81%   46.78%   -0.03%     
==========================================
  Files         165      165              
  Lines        7763     7763              
  Branches       24       24              
==========================================
- Hits         3634     3632       -2     
- Misses       3907     3910       +3     
+ Partials      222      221       -1
Impacted Files Coverage Δ
src/app/backend/sync/secret.go 70.29% <0%> (-2.98%) ⬇️
...p/backend/integration/metric/common/aggregation.go 90.9% <0%> (+1.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6edc1da...d9ca979. Read the comment docs.

@ajatprabha
Copy link
Contributor Author

As discussed on Slack, I have added the status bar on each list component. This allowed using existing data in the component as suggested by @maciaszczykm. cc: @jeefy

@jeefy
Copy link
Member

jeefy commented Mar 9, 2019

I tested this and things look good.

@ajatprabha Is this still considered a WIP? Or is this PR ready for review?

@ajatprabha
Copy link
Contributor Author

ajatprabha commented Mar 9, 2019

I was thinking to also add PodList.cumulativeMetrics to this PR. I'll open a new PR for that and remove WIP from this one.

@ajatprabha ajatprabha changed the title [WIP] Workloads status component for overview page Workloads status component for overview page Mar 9, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 9, 2019
@ajatprabha ajatprabha changed the title Workloads status component for overview page Workloads status component for resource list components Mar 9, 2019
@floreks
Copy link
Member

floreks commented Mar 11, 2019

IMHO adding a status bar on top of every list makes everything a bit overwhelming. Lists should be clean and contain only key information about the objects. Adding such bars might distract the user. I'd stick to the original charts at the top of the page to show statuses. List itself already contains status for every object. We should not need any more indicators to keep it as simple and clean as possible.

@maciaszczykm
Copy link
Member

maciaszczykm commented Mar 11, 2019

I generally like the idea of having additional information next to the lists but I think we could use other design here. At the moment it doesn't match the styling of the rest of the component, is a bit too bold and colors are a bit overwhelming. We could use the help of someone who knows more than us about design, @danielromlein do you have some time for it?

I think we will still need a component to display statuses of all workloads in one place. This (in each card) might be some feature enabled in the settings once we will style it. Both have their advantages, one is easier to reach, the second gives a better overview.

Additionally like @floreks noticed we have a lot of things in the card components, we have to be careful here.

@ajatprabha
Copy link
Contributor Author

I had the same thought as @floreks while implementing this.

How do I pull data for a central component from backend then? Suggestions would be helpful. A new endpoint in the backend or maybe get it from state?

@maciaszczykm
Copy link
Member

maciaszczykm commented Mar 11, 2019

Is calling all workload endpoints an option? Then we can send multiple calls asynchronously and display graphs when the responses come.

@ajatprabha
Copy link
Contributor Author

ajatprabha commented Mar 11, 2019

We'll be replicating calls that way. Maybe we can use ViewChild to get data from list component or use Output?

Is there a Service which manages this data from backend? I think component calls the endpoint itself right now.
I know about the EndpointManager but that only provides endpoints right?

@maciaszczykm
Copy link
Member

I know, output was the thing that I was mentioning before on Slack. The problem with this is coupling components. With separate calls it is independent. @floreks WDYT?

@floreks
Copy link
Member

floreks commented Mar 11, 2019

How about adding emitter to the list and emit data on list update? This way we could avoid hard coupling and other components that require similar data could easily subscribe to it. I don't remember exactly how backend looked like before. I'd have to check it.

@maciaszczykm
Copy link
Member

@floreks We had group resources, i.e. workloads, overview, config etc.

@ajatprabha
Copy link
Contributor Author

ajatprabha commented Mar 17, 2019

Closing in favour of #3667

@ajatprabha ajatprabha closed this Mar 17, 2019
@ajatprabha ajatprabha deleted the workload-statuses branch March 17, 2019 13:24
@ajatprabha ajatprabha restored the workload-statuses branch March 17, 2019 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants