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

GithubActions Logging Implementation #2172

Merged
merged 17 commits into from Sep 12, 2020
Merged

GithubActions Logging Implementation #2172

merged 17 commits into from Sep 12, 2020

Conversation

NetPenguins
Copy link
Contributor

@NetPenguins NetPenguins commented Aug 29, 2020

Hey, I just made a Pull Request!

Pull request for #1858

  • Added WorkflowRunLogs
  • Updated WorkflowRunDetails to display WorkflowRunLogs element per job.
  • Extended GithubActions* with downloadJobLogsForWorkflowRun

WorkflowRunDetails will pass in a Job object to WorkflowRunLogs element. We then use WorkflowRunLogs to find the associated log for the passed in job. Once we have the run we will create a new Accordion element to house a LazyLog component and a Modal component. The user can either choose to expand the Accordion to view the logs within the context of the parent object, or click the Open Logs icon to the right to open the Modal. In both cases the AccordionSummary and Modal will house a DisplayLog object.

In the event the run is still in progress or queued we will disable the associated Accordion object.

CircularProgress was added to the Accordion element.

Syntax highlighting is used to identify if the line in LazyLog message contains error, failed or failure and changes the font color to red for easier identification of errors. Errors will also cause the boxShadow to turn red and displays a red icon.

Screen Shot 2020-09-01 at 2 22 28 PM
Screen Shot 2020-09-01 at 2 22 50 PM
Screen Shot 2020-09-01 at 2 22 58 PM
Screen Shot 2020-09-01 at 2 23 09 PM

✔️ 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

@NetPenguins NetPenguins requested a review from a team as a code owner August 29, 2020 14:27
@NetPenguins NetPenguins marked this pull request as draft August 30, 2020 12:26
@NetPenguins
Copy link
Contributor Author

Mistake in using outdated repo. . . revising now.

@shmidt-i
Copy link
Contributor

Regarding repo - you can use spotify/backstage itself, has a lot of workflows and (only sometimes 😉 ) failures

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.

Nice job! Some comments below:

owner: string;
repo: string;
runId: number;
}): Promise<any> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you avoid any here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call out. I plugged in Promise<EndpointInterface> and have good results. Will place in the update.

ExpansionPanelSummary,
} from '@material-ui/core';

import React, {Suspense} 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.

Looks like prettier wasn't ran here

Comment on lines 73 to 81
if (!entityCompoundName.name) {
// TODO(shmidt-i): remove when is fully integrated
// into the entity view
entityCompoundName = {
kind: 'Component',
name: 'backstage',
namespace: 'default',
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Soon will be gone, but nothing to do now ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaand since #2076 was merged, this is subject to be redone similar to
https://github.com/spotify/backstage/blob/8888b0c801002dd4112615955c2a2799f32015a9/plugins/github-actions/src/components/WorkflowRunsTable/WorkflowRunsTable.tsx#L157

Ping me on discord if you have any questions, will be happy to help you out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like the new look!👌 I will look over the changes from what I see it's basically just removing the entityCompoundName in favor of the imported Entity but I'll reach out if I run into any questions

}}
>
<Typography variant="button">
{jobLogs.value === null ? "No Logs to Display" : "Job Log"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Some loading state here would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this one I just tested while a build was in progress and see the issue with this currently. I will update to check for in progress or not and display accordingly.

@NetPenguins NetPenguins marked this pull request as ready for review September 1, 2020 18:34
@freben
Copy link
Member

freben commented Sep 3, 2020

Just a small conflict left! (oh I also saw the comment about simplified hook for getting the project name) Let us know if we can help out.

@stefanalund
Copy link
Contributor

Fixes #2238

@NetPenguins
Copy link
Contributor Author

@shmidt-i @freben Resolved the yarn.lock conflicts and have incorporated the new entity usage. I am running into an issue where yarn install fails during the checks due to Cypress. Have you come across this issue before?

@shmidt-i
Copy link
Contributor

shmidt-i commented Sep 7, 2020

It seems like it happens from time to time, was able to find this issue cypress-io/cypress#6789

@freben
Copy link
Member

freben commented Sep 7, 2020

Good find - that's a crappy bug cypress bug...

Will you retrigger the workflows?

yarn.lock Outdated
Comment on lines 8428 to 8431
cypress@*:
version "5.0.0"
resolved "https://registry.npmjs.org/cypress/-/cypress-5.0.0.tgz#6957e299b790af8b1cd7bea68261b8935646f72e"
integrity sha512-jhPd0PMO1dPSBNpx6pHVLkmnnaTfMy3wCoacHAKJ9LJG06y16zqUNSFri64N4BjuGe8y6mNMt8TdgKnmy9muCg==
Copy link
Contributor

Choose a reason for hiding this comment

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

It may have something to do with this addition

Copy link
Member

Choose a reason for hiding this comment

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

Yeah likely, can you make it resolve to the same version as the other one?

Copy link
Member

Choose a reason for hiding this comment

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

The yarn.lock changes should be removed from this PR, except for the lazylog addition

@shmidt-i
Copy link
Contributor

shmidt-i commented Sep 7, 2020

Good find - that's a crappy bug cypress bug...

Will you retrigger the workflows?

I tried, still failing. Let's see tomorrow, maybe time heals. Or will investigate more why this 5.0 version appeared 🤷‍♂️

@freben
Copy link
Member

freben commented Sep 7, 2020

Mm, the ticket you linked describes it (or one that it in turn links to perhaps?). I wonder if the dedupe command actually removes it too and makes it point to 4.x like the other one

@NetPenguins
Copy link
Contributor Author

All updates made and conflicts resolved.

@shmidt-i
Copy link
Contributor

Very good, thanks! 🎉

@shmidt-i shmidt-i merged commit 4780043 into backstage:master Sep 12, 2020
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.

None yet

5 participants