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 propsector code analysis to CI run #1021

Closed

Conversation

vickywane
Copy link

This pull request will fix this issue.

This pull request adds Prospector as a static code analysis tool to run an analysis each time a PR is opened through the use of GitHub actions.

@vickywane vickywane force-pushed the feat/static-code-analysis branch from f404416 to a4d884a Compare October 16, 2022 22:17
@vickywane
Copy link
Author

At this point, Prospector has been set up with a very minimal configuration and it stores the result using GitHub Action Artifacts. We can proceed to adjust its configuration to better suit the project.

@paxcema Please review.

Copy link
Contributor

@paxcema paxcema left a comment

Choose a reason for hiding this comment

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

Hey @vickywane,

Looks like an interesting proposal. I think it's still missing a way for us to leverage this in an automated fashion when contributors open new pull requests, because I don't think this configuration goes beyond writing the report internally in the CI machines, is this correct? If so, then I think we need to figure out a way to achieve that before merging this.

@vickywane
Copy link
Author

Hello @paxcema -- Thanks for the response here.

I agree with your thoughts on this. However, there is some unclarity on what exactly is being expected.
Currently, the CI in the PR is set up to run the static analysis, but I have no idea what the team wants to do with the result of the analysis.

One thing that I can think of is to paste the result of the CI run into the PR that is opened. ( Although this might result into noise within the PR)

Do let me know what you think.

@paxcema
Copy link
Contributor

paxcema commented Oct 20, 2022

what exactly is being expected

Largely what the original issue states: a way for us to automatically trigger a bot in new PRs that offers static analysis (yes, will be noisy but should be fine as long as the report is actually useful to either/both maintainer and contributors).

Prospector is a good choice. I think we can configure it to check for our current flake8 rules, plus anything else that makes sense and is easy to add. After that, the report should automatically be added to the PR discussion with an easy-to-read markdown format. And I think after the initial report, ideally a contributor would comment some special string to retrigger the check, though this may be too complicated and best left for a second PR.

Does this make sense?

@vickywane
Copy link
Author

Thank you for explaining @paxcema.

I'll make an update to the setup.

1 similar comment
@vickywane
Copy link
Author

Thank you for explaining @paxcema.

I'll make an update to the setup.

@vickywane
Copy link
Author

@paxcema I have made an update on this PR.

Here are my updates:

  • I have re-configured Prospector to generate YAML content for the static analysis. Outputting the analysis in Markdown format is not supported by the prospector tool.

  • Prospector doesn't have support for Flake8. What I have done is to re-configure it to have similar behavior as the .flake8 file in ignoring certain paths and the max-line-length. The flake8 rules are done with Prospector's strictness levels. It would be great if you could take a look at the current logs from Prospector and let me know if I should increase or reduce the current strictness

  • I added the thollander/actions-comment-pull-request@v1 action to paste the YAML content of the analysis here but that seems to have a permissions-related blocker. thollander/actions-comment-pull-request@v1 requires a GITHUB_TOKEN in order to be able to make a comment, but I don't have access to the token in as a contributor. From the research done, that token will be available after the PR has been merged into the main branch. See this issue on this.

@vickywane vickywane requested a review from paxcema October 21, 2022 12:14
Copy link
Contributor

@paxcema paxcema left a comment

Choose a reason for hiding this comment

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

This is looking great @vickywane!

I'd say we can go ahead, merge and check how it behaves. @ZoranPandovski what do you think?

@paxcema
Copy link
Contributor

paxcema commented Nov 3, 2022

@vickywane while we wait for the other reviewer, I noticed the linting check is now throwing a lot of warnings, as if the flake8 configuration had been change in the linting step, as well. Any clues as to why that may be happening? It is a blocker for merging, I'm afraid.

@@ -0,0 +1 @@
prospector==1.7.7
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I don't think we need this right? The CI script manually installs it, anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is installed in the gh action env

@paxcema
Copy link
Contributor

paxcema commented Mar 29, 2023

@vickywane any updates? Also, @ZoranPandovski it would be awesome if you can review this

@@ -0,0 +1,35 @@
name: Static Code Analysis Of Codebase
on: [push, pull_request]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe for now we can just run this on pull_request

@@ -0,0 +1 @@
prospector==1.7.7
Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is installed in the gh action env

@ZoranPandovski
Copy link
Member

Thanks @vickywane. Please just address the above two comments.

@paxcema
Copy link
Contributor

paxcema commented May 5, 2023

Closing this for now. Feel free to reopen.

@paxcema paxcema closed this May 5, 2023
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.

3 participants