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 analysis modal #3174

Merged
merged 6 commits into from
Dec 18, 2023
Merged

Conversation

emily-blixt-ck
Copy link
Contributor

@emily-blixt-ck emily-blixt-ck commented Nov 16, 2023

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.

  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".

  • I've signed my commits with DCO

  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.

  • My builds are green. Try syncing with master if they are not.

  • My organization is added to USERS.md.

Screenshots:

Summary panel
Screenshot 2023-11-16 at 4 26 44 PM

Metric chart demonstrating successes and errors
Screenshot 2023-11-16 at 2 48 15 PM

Table view demonstrating successes and errors
Screenshot 2023-11-16 at 2 48 23 PM

Successful metric chart
Screenshot 2023-11-16 at 2 49 25 PM

Failed metric table
Screenshot 2023-11-16 at 2 52 17 PM

Copy link
Contributor

github-actions bot commented Nov 16, 2023

Go Published Test Results

2 092 tests   2 092 ✔️  2m 49s ⏱️
   118 suites         0 💤
       1 files           0

Results for commit f8cf2c0.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 17, 2023

E2E Tests Published Test Results

    4 files      4 suites   3h 34m 53s ⏱️
106 tests   93 ✔️   6 💤   7
444 runs  400 ✔️ 24 💤 20

For more details on these failures, see this check.

Results for commit f8cf2c0.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3a257e3) 81.81% compared to head (f8cf2c0) 81.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3174      +/-   ##
==========================================
+ Coverage   81.81%   81.83%   +0.01%     
==========================================
  Files         135      135              
  Lines       20629    20629              
==========================================
+ Hits        16878    16881       +3     
+ Misses       2880     2877       -3     
  Partials      871      871              

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

@ashutosh16
Copy link
Contributor

ashutosh16 commented Dec 5, 2023

@emily-blixt-ck Analysis tab needs to be color code based on the health status. For e.g.
Degraded - Red
Healthy - Green
Running - Blue
Inconclusive - yellow
Screenshot 2023-12-05 at 9 46 22 AM

@@ -0,0 +1,79 @@
import * as React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it better to rename the index.tsx to analysis-modal.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense for consistency. I renamed it.

@emily-blixt-ck
Copy link
Contributor Author

@emily-blixt-ck Analysis tab needs to be color code based on the health status. For e.g. Degraded - Red Healthy - Green Running - Blue Inconclusive - yellow Screenshot 2023-12-05 at 9 46 22 AM

Thanks for pointing this out! Added a simple status indicator (this doesn't include a sub-status dot, but we could add that too if we want).

Screenshot 2023-12-05 at 1 08 32 PM

Signed-off-by: Emily <[email protected]>
@@ -0,0 +1,2 @@
export {default as AnalysisPanel} from './analysis-panel';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is the Summary Panel that you see when you open the modal. Now that I've typed that... I could rename it to SummaryPanel if that would be more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, renaming would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@ashutosh16 ashutosh16 left a comment

Choose a reason for hiding this comment

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

LGTM

@ashutosh16
Copy link
Contributor

@zachaller Would you be able to run this locally and verify the UI?

zachaller and others added 2 commits December 13, 2023 16:19
Copy link

sonarcloud bot commented Dec 14, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
1 Security Hotspot
No data about Coverage
2.3% Duplication on New Code

See analysis details on SonarCloud

@zachaller zachaller merged commit 0eff316 into argoproj:master Dec 18, 2023
23 checks passed
@zachaller
Copy link
Collaborator

Thanks for all the hard work! This is a great addition!

@danil-smirnov
Copy link
Contributor

We are waiting for this to be released folks!

@zachaller
Copy link
Collaborator

These are the last features that need to make it in for 1.7 then I should be good to cut a 1.7 release, I will try to get to them soon.

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.

4 participants