Skip to content

[INFRA] Add Dependency Review step to prevent introducing vulnerable dependencies#4266

Closed
bowenliang123 wants to merge 1 commit intoapache:masterfrom
bowenliang123:dependency-review
Closed

[INFRA] Add Dependency Review step to prevent introducing vulnerable dependencies#4266
bowenliang123 wants to merge 1 commit intoapache:masterfrom
bowenliang123:dependency-review

Conversation

@bowenliang123
Copy link
Contributor

@bowenliang123 bowenliang123 commented Feb 7, 2023 •
edited
Loading

Why are the changes needed?

image

How was this patch tested?

  • Pass the Dependency Check CI job

@github-actions github-actions bot added the kind:infra license, community building, project builds, asf infra related, etc. label Feb 7, 2023
@bowenliang123 bowenliang123 changed the title add actions/dependency-review-action to dep workflow add dependency review step to dependency workflow Feb 7, 2023
@github-actions github-actions bot removed the kind:infra license, community building, project builds, asf infra related, etc. label Feb 7, 2023
@bowenliang123 bowenliang123 deleted the dependency-review branch February 7, 2023 16:15
@bowenliang123 bowenliang123 restored the dependency-review branch February 7, 2023 16:20
@bowenliang123 bowenliang123 reopened this Feb 7, 2023
@github-actions github-actions bot added the kind:infra license, community building, project builds, asf infra related, etc. label Feb 7, 2023
@bowenliang123 bowenliang123 changed the title add dependency review step to dependency workflow [INFRA] Add Dependency Review step to prevent introducing vulnerable dependencies Feb 8, 2023
@bowenliang123 bowenliang123 marked this pull request as ready for review February 8, 2023 09:59
yaooqinn
yaooqinn reviewed Feb 9, 2023
View reviewed changes
.github/workflows/dep.yml Outdated
- name: Dependency Review
uses: actions/dependency-review-action@v3
with:
fail-on-severity: low
Copy link
Member

@yaooqinn yaooqinn Feb 9, 2023

Choose a reason for hiding this comment

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

Is low too strict? Use moderate?

Copy link
Contributor Author

@bowenliang123 bowenliang123 Feb 9, 2023

Choose a reason for hiding this comment

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

The docs of action does not clarify how it classify the level of severity, so I prefer to keep it in low the same as the default value. We could lower this level if necessary in future, as it only affects future PRs.

Copy link
Member

@yaooqinn yaooqinn Feb 9, 2023

Choose a reason for hiding this comment

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

The risk for vulnerability is determined by the CVSS score. If you go to check the mvn central or NVD, it's common to see an artifact have some low-level vulnerabilities. I guess it is not practical for us to add such a critical rule that blocks PRs frequently

Copy link
Contributor Author

@bowenliang123 bowenliang123 Feb 9, 2023

Choose a reason for hiding this comment

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

OK, changed to moderate.

yaooqinn
yaooqinn reviewed Feb 9, 2023
View reviewed changes
@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2023 •
edited
Loading

Codecov Report

Merging #4266 (e11fad0) into master (95318d5) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head e11fad0 differs from pull request most recent head 63a84b2. Consider uploading reports for the commit 63a84b2 to get more accurate results

@@             Coverage Diff              @@
##             master    #4266      +/-   ##
============================================
+ Coverage     53.38%   53.40%   +0.01%     
  Complexity       13       13              
============================================
  Files           560      560              
  Lines         30562    30562              
  Branches       4139     4139              
============================================
+ Hits          16315    16321       +6     
+ Misses        12711    12708       -3     
+ Partials       1536     1533       -3
Impacted Files Coverage Δ
.../engine/spark/session/SparkSQLSessionManager.scala 78.48% <0.00%> (-1.27%) ⬇️
...apache/kyuubi/engine/JpsApplicationOperation.scala 77.41% <0.00%> (ø)
...rc/main/scala/org/apache/spark/ui/EnginePage.scala 79.26% <0.00%> (+0.28%) ⬆️
...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala 79.01% <0.00%> (+0.61%) ⬆️
...mon/src/main/scala/org/apache/kyuubi/Logging.scala 42.50% <0.00%> (+1.25%) ⬆️
...ache/kyuubi/operation/KyuubiOperationManager.scala 82.66% <0.00%> (+2.66%) ⬆️
...uubi/engine/spark/events/SparkOperationEvent.scala 94.44% <0.00%> (+5.55%) ⬆️

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

@bowenliang123 bowenliang123 self-assigned this Feb 9, 2023
@bowenliang123 bowenliang123 added this to the v1.7.0 milestone Feb 9, 2023
@bowenliang123 bowenliang123 deleted the dependency-review branch February 9, 2023 06:15
@bowenliang123
Copy link
Contributor Author

bowenliang123 commented Feb 9, 2023

Thanks, merged to master.

@cxzl25 cxzl25 mentioned this pull request Jun 26, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yaooqinn yaooqinn yaooqinn approved these changes

Assignees

@bowenliang123 bowenliang123

Labels

kind:infra license, community building, project builds, asf infra related, etc.

Projects

None yet

Milestone

v1.7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@bowenliang123 @codecov-commenter @yaooqinn