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

Integrate Sonarcloud and Nancy github action #3491

Merged
merged 22 commits into from
Sep 14, 2022

Conversation

wryonik
Copy link
Contributor

@wryonik wryonik commented Mar 28, 2022

Related issue

#2617

Milestone of this PR

What type of PR is this

/kind enhancement

Proposed Changes

Integrate Sonarcloud for static code analysis and Nancy for analysing dependencies

Proof Manifests

Checklist

  • I have read the contributing guidelines.
  • I have read the PR documentation guide and followed the process including adding proof manifests to this PR.
  • This is a bug fix and I have added unit tests that prove my fix is effective.
  • This is a feature and I have added CLI tests that are applicable.
  • My PR needs to be cherry picked to a specific release branch which is .
  • My PR contains new or altered behavior to Kyverno and
    • CLI support should be added and my PR doesn't contain that functionality.
    • I have added or changed the documentation myself in an existing PR and the link is:
    • I have raised an issue in kyverno/website to track the documentation update and the link is:

Further Comments

There are certain things that are needed to be set for this project:-

  • Create a project on sonarcloud and set the organisation as "kyverno" and project as "kyverno" (else any changes should be updated in the sonar-project.properties files as well).
  • Create 2 github secrets for this repo:
    • ACCESS_TOKEN - Github access token
    • SONAR_TOKEN - this is to be generated from the sonarcloud dashboard

A testing version can be accessed here (for Sonarcloud) and here (for Nancy)

Integrate Sonarcloud for static code analysis and Nancy for analysing
dependencies

Signed-off-by: Shubham Gupta <shubham.gupta2956@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2022

Codecov Report

Merging #3491 (ffb57de) into main (530a584) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3491   +/-   ##
=======================================
  Coverage   30.69%   30.69%           
=======================================
  Files         167      167           
  Lines       20505    20505           
=======================================
  Hits         6295     6295           
  Misses      13461    13461           
  Partials      749      749           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@realshuting
Copy link
Member

@wryonik - the two new tests failed, can you please take a look?

Copy link
Contributor

@prateekpandey14 prateekpandey14 left a comment

Choose a reason for hiding this comment

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

@wryonik - there are some failure in Sonar workflow looking for SONAR_TOKEN , do we need to configure that ?

@wryonik
Copy link
Contributor Author

wryonik commented Apr 20, 2022

Yes @prateekpandey14. There is a SONAR_TOKEN variable which is required for the workflow. @JimBugwadia set it few days back but I guess it might not be set properly.

@chipzoller
Copy link
Member

What's needed to close this out?

@eddycharly
Copy link
Member

eddycharly commented Aug 23, 2022

@realshuting @JimBugwadia @prateekpandey14 @vyankyGH shall we merge this ?
Could be worth a try, we can always remove them if this is not wall suited for our needs.

@chipzoller
Copy link
Member

This has been outstanding for too long for such a minor addition. Are we adding or rejecting?

@realshuting
Copy link
Member

Some of the tests failed, @wryonik - are you able to take a look and fix them?

@eddycharly
Copy link
Member

Folks, this is 6 months old. I can fix the issues if we want to keep it.

@realshuting
Copy link
Member

Folks, this is 6 months old. I can fix the issues if we want to keep it.

Yes @eddycharly - let's fix and merge the PR.

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>
@eddycharly
Copy link
Member

@realshuting this should be good 🤞

@realshuting
Copy link
Member

Nancy and sonarcloud checks failed, are they required?

@eddycharly
Copy link
Member

Nancy and sonarcloud checks failed, are they required?

They can't succeed, we don't share secrets with PRs coming from forks.

@realshuting
Copy link
Member

Nancy and sonarcloud checks failed, are they required?

They can't succeed, we don't share secrets with PRs coming from forks.

ok, then do we want to enable them for PRs?

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>
Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>
@eddycharly
Copy link
Member

ok, then do we want to enable them for PRs?

removed

Copy link
Member

@realshuting realshuting left a comment

Choose a reason for hiding this comment

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

Thanks @eddycharly !

@realshuting realshuting enabled auto-merge (squash) September 14, 2022 06:11
@realshuting realshuting merged commit f00c12e into kyverno:main Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants