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

Run sonar scan in separate job #322

Merged
merged 10 commits into from
Jul 13, 2022
Merged

Run sonar scan in separate job #322

merged 10 commits into from
Jul 13, 2022

Conversation

NSeydoux
Copy link
Contributor

This allows to run add sonar-specific conditions (e.g. not running the scan with dependabot) at a job level, rather than doing it for each step.

This allows to run add sonar-specific conditions (e.g. not running the scan with dependabot) at a job level, rather than doing it for each step.
@vercel
Copy link

vercel bot commented Jul 12, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
solid-client-notifications-js ✅ Ready (Inspect) Visit Preview Jul 13, 2022 at 7:41AM (UTC)

@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 12, 2022 09:39 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 12, 2022 09:39 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 12, 2022 09:39 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 12, 2022 09:39 Inactive
@vercel vercel bot temporarily deployed to Preview July 12, 2022 09:40 Inactive
@NSeydoux NSeydoux changed the title Run sonar scan is separate job Run sonar scan in separate job Jul 12, 2022
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 12, 2022 09:45 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 12, 2022 09:45 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 12, 2022 09:45 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 12, 2022 09:45 Inactive
@vercel vercel bot temporarily deployed to Preview July 12, 2022 09:46 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 12, 2022 09:51 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 12, 2022 09:51 Inactive
@NSeydoux NSeydoux marked this pull request as ready for review July 12, 2022 09:51
@NSeydoux NSeydoux requested a review from a team as a code owner July 12, 2022 09:51
@NSeydoux NSeydoux requested a review from edwardsph July 12, 2022 09:51
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 12, 2022 10:03 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 12, 2022 10:03 Inactive
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@edwardsph edwardsph left a comment

Choose a reason for hiding this comment

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

I agree the upload is still conditional, so have we really gained enough? The separation is good but we still have a conditional in the main workflow steps and we've added the artifact upload, download and an additional checkout.
I really appreciate the effort in getting Sonar embedded into the CI process and investigating these options, but ultimately, I think it is up to the devtools team to decide what suits you best for maintaining it.

@vercel vercel bot temporarily deployed to Preview July 12, 2022 11:10 Inactive
@NSeydoux
Copy link
Contributor Author

I would say that moving sonar to an isolated job is a first step towards moving it to a shared workflow, so I think even if the gain is minimal here, it is going in the right direction.

@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 12, 2022 11:23 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 12, 2022 11:23 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 12, 2022 11:23 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 12, 2022 11:23 Inactive
@vercel vercel bot temporarily deployed to Preview July 12, 2022 11:24 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 12, 2022 11:28 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 12, 2022 11:28 Inactive
Copy link
Contributor

@edwardsph edwardsph left a comment

Choose a reason for hiding this comment

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

LGTM with minor suggestion

Comment on lines +39 to +40
# Sonar analysis needs the full history for features like automatic assignment of bugs. If the following step
# is not included the project will show a warning about incomplete information.
Copy link
Contributor

Choose a reason for hiding this comment

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

I was suggesting a minor change to this comment as it no longer refers to a separate step, just the parameter. Perhaps instead of my previous suggestion, the simplest change would be from following step to following option.

Copy link
Contributor

@ThisIsMissEm ThisIsMissEm left a comment

Choose a reason for hiding this comment

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

One minor improvement, but LGTM

- uses: actions/upload-artifact@v3
if: ${{ matrix.os == 'ubuntu-latest' && matrix.node-version == '16.x' }}
with:
name: code-coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps:

Suggested change
name: code-coverage
name: code-coverage-${matrix.os}-${matrix.node-version}

@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 13, 2022 07:33 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 13, 2022 07:33 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 13, 2022 07:33 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 13, 2022 07:33 Inactive
@vercel vercel bot temporarily deployed to Preview July 13, 2022 07:34 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 13, 2022 07:40 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces July 13, 2022 07:40 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 13, 2022 07:40 Inactive
@NSeydoux NSeydoux temporarily deployed to ESS PodSpaces Next July 13, 2022 07:40 Inactive
@vercel vercel bot temporarily deployed to Preview July 13, 2022 07:41 Inactive
@NSeydoux NSeydoux merged commit 630a000 into main Jul 13, 2022
@NSeydoux NSeydoux deleted the chore/run-sonar-dedicated-job branch July 13, 2022 07:54
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