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

fix: Expose cluster information for Workflow Executions #78

Merged
merged 8 commits into from
Jul 1, 2020

Conversation

schottra
Copy link
Contributor

@schottra schottra commented Jun 30, 2020

The primary goal was to expose a piece of metadata that is useful in debugging. But since the portion of the page where we show execution details is already pretty crowded, I decided to implement a restyling that would be part of some upcoming design changes for the execution details page. This gave me more room to add the extra piece of information

  • Moved most details out of header into a separate section below the header, which is collapsible
  • Updated execution name to render all as one string, making it easier to copy
  • Updated styling of details
  • Added 'Cluster' detail. Executions with no spec.metadata.systemMetadata.executionClusterName property will just show "(unknown)"
  • Added errors for WorkflowExecutions, shown at the bottom of the metadata section

Before:

image

After:

image

Expansions:
ExpandCollapseExecutionMetadata

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2020

Codecov Report

Merging #78 into master will increase coverage by 0.17%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   62.88%   63.06%   +0.17%     
==========================================
  Files         361      363       +2     
  Lines        5647     5685      +38     
  Branches      837      840       +3     
==========================================
+ Hits         3551     3585      +34     
- Misses       2096     2100       +4     
Impacted Files Coverage Δ
...ExecutionDetails/ExecutionDetailsAppBarContent.tsx 47.50% <50.00%> (+5.83%) ⬆️
...s/Executions/ExecutionDetails/ExecutionDetails.tsx 65.51% <66.66%> (+0.51%) ⬆️
.../Executions/ExecutionDetails/ExecutionMetadata.tsx 100.00% <100.00%> (ø)
...Executions/ExecutionDetails/ExecutionNodeViews.tsx 60.00% <100.00%> (+1.66%) ⬆️
...omponents/Executions/ExecutionDetails/constants.ts 100.00% <100.00%> (ø)
src/components/Theme/constants.ts 100.00% <100.00%> (ø)
...nts/Executions/Tables/ExpandableExecutionError.tsx 57.14% <0.00%> (-42.86%) ⬇️
src/components/common/ExpandableMonospaceText.tsx 52.38% <0.00%> (-38.10%) ⬇️
.../components/Executions/Tables/NodeExecutionRow.tsx 88.88% <0.00%> (-2.23%) ⬇️
... and 2 more

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 bdac7ed...2c012bb. Read the comment docs.

BobNisco
BobNisco previously approved these changes Jun 30, 2020
Copy link
Contributor

@BobNisco BobNisco left a comment

Choose a reason for hiding this comment

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

Some minor suggestions inline, but LGTM 👍

podehnal
podehnal previously approved these changes Jun 30, 2020
Copy link

@podehnal podehnal left a comment

Choose a reason for hiding this comment

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

👍

@schottra schottra dismissed stale reviews from podehnal and BobNisco via a96b805 July 1, 2020 18:05
@service-github-lyft-semantic-release
Copy link
Contributor

🎉 This PR is included in version 0.7.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

5 participants