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: add events page #628

Merged
merged 11 commits into from
Jun 22, 2020
Merged

feat: add events page #628

merged 11 commits into from
Jun 22, 2020

Conversation

g1eny0ung
Copy link
Member

What problem does this PR solve?

As the title.

@codecov-commenter
Copy link

codecov-commenter commented Jun 15, 2020

Codecov Report

Merging #628 into master will decrease coverage by 0.17%.
The diff coverage is 21.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
- Coverage   55.78%   55.61%   -0.18%     
==========================================
  Files          68       68              
  Lines        4383     4393      +10     
==========================================
- Hits         2445     2443       -2     
- Misses       1768     1779      +11     
- Partials      170      171       +1     
Impacted Files Coverage Δ
controllers/networkchaos/ipset/ipset.go 37.14% <0.00%> (ø)
pkg/store/experiment/experiment.go 32.20% <0.00%> (-6.58%) ⬇️
pkg/utils/chaosdaemon.go 51.21% <0.00%> (ø)
pkg/utils/grpc.go 0.00% <0.00%> (ø)
controllers/networkchaos/netem/types.go 47.27% <50.00%> (ø)
controllers/podchaos/containerkill/types.go 64.38% <100.00%> (ø)
controllers/timechaos/types.go 61.44% <100.00%> (ø)
pkg/utils/selector.go 51.42% <0.00%> (-0.82%) ⬇️

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 a26f169...76789e0. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2020

This pull request introduces 2 alerts when merging b65ea84 into a26f169 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2020

This pull request introduces 2 alerts when merging cf6a3b6 into a26f169 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

ui/src/components/EventsTable/index.tsx Outdated Show resolved Hide resolved
ui/src/components/EventsTable/index.tsx Outdated Show resolved Hide resolved
@yeya24
Copy link
Contributor

yeya24 commented Jun 15, 2020

I got a bug while I am using, but I am not sure what's the cause in the code.

Screenshot from 2020-06-15 11-40-57
The experiment aaa was deleted several minutes ago. But I can still see this in the chart.

@g1eny0ung
Copy link
Member Author

I got a bug while I am using, but I am not sure what's the cause in the code.

Screenshot from 2020-06-15 11-40-57
The experiment aaa was deleted several minutes ago. But I can still see this in the chart.

I also encountered this problem. Seems it's a bug of the events API.

@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2020

This pull request introduces 2 alerts when merging 5647ea8 into a26f169 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@yeya24
Copy link
Contributor

yeya24 commented Jun 15, 2020

I got a bug while I am using, but I am not sure what's the cause in the code.
Screenshot from 2020-06-15 11-40-57
The experiment aaa was deleted several minutes ago. But I can still see this in the chart.

I also encountered this problem. Seems it's a bug of the events API.

Will investigate this today. Good work!

@lgtm-com
Copy link

lgtm-com bot commented Jun 16, 2020

This pull request introduces 2 alerts when merging 44ce709 into a26f169 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 16, 2020

This pull request introduces 2 alerts when merging f3ddbd1 into 7eb8d81 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@g1eny0ung
Copy link
Member Author

I append some patches to this PR, includes:

  • Now can pause and restart an experiment
  • Add detail link button at the end of each row of the events table
  • Show pods info in the events table

@g1eny0ung g1eny0ung requested review from yeya24 and cwen0 June 16, 2020 08:23
@lgtm-com
Copy link

lgtm-com bot commented Jun 16, 2020

This pull request introduces 2 alerts when merging fd35fa1 into 7eb8d81 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@yeya24
Copy link
Contributor

yeya24 commented Jun 16, 2020

@g1eny0ung Would you mind taking a look at the lgtm bot? Can you solve this?

@g1eny0ung
Copy link
Member Author

@g1eny0ung Would you mind taking a look at the lgtm bot? Can you solve this?

I viewed its warning notification several hours ago. its actually "used", just the variable definition not used.

The reason I wanna keep it because it can help other people to understand what this code does if he wants to deep into it.

As u can see, I already put a comment // eslint-disable-next-line on the top of them. ESLint is a JS linter which is widely used in front-end, It has checked these codes (plays the same role as the bot).

I suggest banning the bot from checking TS codes because the code has already been linted during development.

@yeya24
Copy link
Contributor

yeya24 commented Jun 16, 2020

I think maybe it is a little weird here because this is a table for events. Do you think it is better to use an Event detail here? Instead of a button for Experiment detail.

Screenshot from 2020-06-16 12-29-07

Another thing is that the events are stored in database, they are records for the past. But the experiment detail API will query from the current cluster, it represents the current status. It is possible that the experiment is not in the cluster anymore.

Screenshot from 2020-06-16 12-32-38

@g1eny0ung
Copy link
Member Author

@yeya24 I think you're right. There is a feature I haven't come true is when clicking the detail button, it will jump to the experiment detail page, I wanna replace the events table in the detail page to an event detail panel.

image

Replace this area to event detail which you selected.

What do you think this way?

@yeya24
Copy link
Contributor

yeya24 commented Jun 17, 2020

@yeya24 I think you're right. There is a feature I haven't come true is when clicking the detail button, it will jump to the experiment detail page, I wanna replace the events table in the detail page to an event detail panel.

image

Replace this area to event detail which you selected.

What do you think this way?

Wow, that's much better!

@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2020

This pull request introduces 2 alerts when merging 7c9817d into 5dd0b86 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@g1eny0ung
Copy link
Member Author

@yeya24 The feature of Event detail already done.

And this thing:

Another thing is that the events are stored in database, they are records for the past. But the experiment detail API will query from the current cluster, it represents the current status. It is possible that the experiment is not in the cluster anymore.

@fewdan will help us filter out the events of survival experiments.

@yeya24
Copy link
Contributor

yeya24 commented Jun 17, 2020

TBH, I think the events table is a little narrow.

Screenshot from 2020-06-17 12-22-30

What's your idea about this status bar, do we need to have this every page? Seems not very important in events page. Especially the New Experiment button.

Screenshot from 2020-06-17 12-23-48

But these things are not very important in this PR. We can improve it later.

@lgtm-com
Copy link

lgtm-com bot commented Jun 18, 2020

This pull request introduces 2 alerts when merging bffa717 into c4672de - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@g1eny0ung
Copy link
Member Author

@yeya24 For cases where the table will be narrow on a small screen. I add a patch commit bffa717.

it will make the left area (which table in) in the detail page from 2/3 to 3/4.

And also, now the sidebar will remember your last choice to initialize its initial width, mini or normal. You can toggle it to mini when your screen is small relatively.

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@zhouqiang-cl
Copy link
Contributor

@cwen0 PTAL

@CLAassistant
Copy link

CLAassistant commented Jun 22, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@cwen0 cwen0 left a comment

Choose a reason for hiding this comment

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

LGTM

@cwen0 cwen0 merged commit a1e7fa4 into chaos-mesh:master Jun 22, 2020
@g1eny0ung g1eny0ung deleted the feat/events branch June 22, 2020 06:29
g1eny0ung referenced this pull request in g1eny0ung/chaos-mesh Jul 2, 2020
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
shonge pushed a commit to shonge/chaos-mesh that referenced this pull request Jul 7, 2020
Signed-off-by: shonge <soog2008@hotmail.com>
@dcalvin dcalvin mentioned this pull request Feb 5, 2021
sjwsl pushed a commit to sjwsl/chaos-mesh that referenced this pull request May 6, 2021
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.

6 participants