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

fix: base-ref/head-ref missed in dependency-review on master #2308

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

msgui
Copy link
Contributor

@msgui msgui commented Sep 9, 2023

Purpose of the PR

fix: base-ref/head-ref miss in dependency-review on pushing to master

Main Changes

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

Comment on lines 3 to 7
on:
push:
branches:
- master
- /^release-.*$/
pull_request:
Copy link
Contributor Author

@msgui msgui Sep 9, 2023

Choose a reason for hiding this comment

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

Base-ref/head-ref are automatically assigned on pull_request, but are missing when pushing to master.
Doc link 👇

image

@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

Merging #2308 (e3d36d1) into master (1d0969d) will increase coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #2308      +/-   ##
============================================
+ Coverage     65.06%   65.08%   +0.01%     
+ Complexity      981      979       -2     
============================================
  Files           498      498              
  Lines         41241    41241              
  Branches       5738     5738              
============================================
+ Hits          26832    26840       +8     
+ Misses        11750    11746       -4     
+ Partials       2659     2655       -4     

see 9 files with indirect coverage changes

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

Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution

@@ -3,7 +3,6 @@ name: "3rd-party"
on:
push:
branches:
- master
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why need to remove it, since it is a triggering branch

Copy link
Contributor Author

@msgui msgui Sep 10, 2023

Choose a reason for hiding this comment

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

I'm not sure why need to remove it, since it is a triggering branch

When pushing to the master branch, base-ref/head-ref isn't auto-assigned, leading to frequent errors. The reason for omitting it is that during PRs, we can directly check for unauthorized third-party libraries using 'Dependency Review' and decide whether to include them. There's no need to showcase this during master branch pushes.
1694346048418
1694346099572

Doc link 👇

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Get it, Thanks for the detailed description

Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

Also update the toolchain & other influenced repo, THX

Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -3,7 +3,6 @@ name: "3rd-party"
on:
push:
branches:
- master
Copy link
Contributor

Choose a reason for hiding this comment

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

Get it, Thanks for the detailed description

@imbajin imbajin changed the title fix: base-ref/head-ref miss in dependency-review on pushing to master fix: base-ref/head-ref missed in dependency-review on master Sep 11, 2023
@imbajin imbajin merged commit 4ceef1a into apache:master Sep 11, 2023
19 of 21 checks passed
DanGuge pushed a commit to DanGuge/incubator-hugegraph that referenced this pull request Sep 25, 2023
VGalaxies pushed a commit to VGalaxies/incubator-hugegraph that referenced this pull request Nov 10, 2023
@msgui msgui deleted the fix-depedency-review branch March 15, 2024 04:09
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