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 widget to show recent git workflow runs #2563

Merged
merged 3 commits into from
Sep 25, 2020

Conversation

iain-b
Copy link
Contributor

@iain-b iain-b commented Sep 22, 2020

Adds a widget to show recent git workflow runs to the github actions plugin. The default setting is the last 5 runs across all branches but both branch and the number of runs are configurable.

https://www.loom.com/share/db09c13f059843ceac09494a7b72199d

Hey, I just made a Pull Request!

✔️ Checklist

  • All tests are passing yarn test
  • Screenshots attached (for UI changes)
  • Relevant documentation updated
  • Prettier run on changed files
  • Tests added for new functionality
  • Regression tests added for bug fixes

@iain-b iain-b requested a review from a team as a code owner September 22, 2020 13:37
render: data => (
<Link
component={RouterLink}
to={generatePath('./ci-cd/:id', { id: data.id! })}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This link assumes that the Entity page in the user's application is configured like the sample app. It could be made more configurable down the line.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly one of the cases we are in the process of discussing how to solve moving forward! cc @Rugvip

Copy link
Member

Choose a reason for hiding this comment

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

Yep! More info about the plan going forward in #2565

@@ -61,14 +62,15 @@ const CICDSwitcher = ({ entity }: { entity: Entity }) => {

const OverviewContent = ({ entity }: { entity: Entity }) => (
<Grid container spacing={3}>
<Grid item>
<Grid item md={6}>
Copy link
Member

Choose a reason for hiding this comment

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

should this entire item be surrounded with essentially if (isJenkinsAvailable(entity) || isGitHubActionsAvailable(entity))? :)

Copy link
Member

Choose a reason for hiding this comment

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

it should most likely not be before the about card in the flow, by the way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted to the way it was before where each widget definition is separate.


beforeEach(() => {
(useWorkflowRuns as jest.Mock).mockReturnValue([{ runs: workflowRuns }]);
(useApi as jest.Mock).mockReturnValue(mockErrorApi);
Copy link
Member

Choose a reason for hiding this comment

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

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

@stefanalund stefanalund left a comment

Choose a reason for hiding this comment

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

Minor nitpick, otherwise :shipit:

export const RecentWorkflowRunsCard = ({
entity,
branch,
dense = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be dense to make it look more like our mockups:

Copy link
Contributor Author

@iain-b iain-b Sep 23, 2020

Choose a reason for hiding this comment

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

Is that dense ? data-wise it's dense but the padding looks "default" (dense is below, looks a little compressed to me)
That said I'd be happy to add fields inline with that mock up in another PR.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@iain-b that does not look better. Let's stick with dense=false.

Copy link
Contributor

@shmidt-i shmidt-i left a comment

Choose a reason for hiding this comment

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

Thanks, looks good, let's nail those small comments and 🚢 it 😄

<JenkinsLatestRunCard branch="master" />
</Grid>
)}
{isGitHubActionsAvailable(entity) && (
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 Author

Choose a reason for hiding this comment

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

@shmidt-i I could write something like:

const RecentCICDRunsSwitcher = ({ entity }: { entity: Entity }) => {
  let content: ReactNode;
  switch (true) {
    case isJenkinsAvailable(entity):
      content = <JenkinsLatestRunCard branch="master" />;
      break;
    case isGitHubActionsAvailable(entity):
      content = <RecentWorkflowRunsCard entity={entity} />;
      break;
    default:
      content = null;
  }
  if (!content) {
    // Don't want to render the grid if there's no matching plugin
    return null;
  }
  return (
    <Grid item sm={6}>
      {content}
    </Grid>
  );
};

What do you think? Seems a little overly specific to me but anything more generic is a bit beyond the scope of this pull request

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

render: data => (
<Link
component={RouterLink}
to={generatePath('./ci-cd/:id', { id: data.id! })}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly one of the cases we are in the process of discussing how to solve moving forward! cc @Rugvip

Adds a widget to show recent git workflow runs to the
github actions plugin. The default setting is the last
5 runs across all branches but both branch and the
number of runs are configurable.
* Fix Entity Page layout
* Fix mock api injection
* Add switcher for recent CI/CD widgets
@iain-b iain-b force-pushed the recent-workflows-widget branch from 12e9166 to 3b2e301 Compare September 24, 2020 09:48
@benjdlambert
Copy link
Member

@shmidt-i can you take another look at this?

Copy link
Contributor

@shmidt-i shmidt-i left a comment

Choose a reason for hiding this comment

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

All good, thanks!

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.

6 participants