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

[YUNIKORN-2098] Change go lint SHA detection (following) #715

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

doupache
Copy link
Contributor

@doupache doupache commented Nov 1, 2023

What is this PR for?

Currently, we will always use the "ORIGIN/HEAD" ref.(In Github Action env) Fallback to "HEAD^" when "ORIGIN/HEAD" doesn't exist.
Also print what git ref the ci have.

I get fatal: Needed a single revision on my fork github action ci.
https://github.com/doupache/yunikorn-k8shim/actions/runs/6713607756/job/18245448950
image

image
image

CI in the forked repo doesn't seem to have the "ORIGIN/HEAD" ref.
I keep getting the 'fatal: Needed a single revision' error, similar to YUNIKORN-285

What type of PR is it?

  • - Bug Fix

What is the Jira issue?

YUNIKORN-2098

@doupache
Copy link
Contributor Author

doupache commented Nov 1, 2023

@wilfred-s Please take a look !

This is the following PR of #210

@targetoee
Copy link
Contributor

targetoee commented Nov 1, 2023

I have the same problem in my fork project.
link: https://github.com/targetoee/yunikorn-k8shim/actions/runs/6716879779/job/18253781242

@doupache
Copy link
Contributor Author

doupache commented Nov 1, 2023

Pass on my fork with branch doupache:YUNIKORN-2098

https://github.com/doupache/yunikorn-k8shim/actions/runs/6714213847

It seems there is another issue with the Pull Request CI environment
I need Committer to approve workflow , help me fix the bug.

@doupache
Copy link
Contributor Author

In YUNIKORN-285, we previously included git symbolic-ref -q HEAD because Travis CI often encountered a detached head situation. However, now that we have switched to GitHub Actions, we can utilize the checkout@v3 action, which efficiently fetches all Git references.

Consequently, we can remove git symbolic-ref -q HEAD Instead, we can initially set REV='origin/HEAD' If git rev-parse fails, we can fall back to REV=HEAD^

Regarding the modification from fetch-depth: 2 to fetch-depth: 0, I have compared the GitHub Action times before this pull request and with the master. The duration of the git checkout action remained around 2 seconds. Therefore, I believe this change is acceptable, as it does not introduce significant overhead.

@wilfred-s I think this PR is ready for review.

Comment on lines +50 to +52
fetch-depth: 0
- name: Fetch all branches and tags
run: git fetch --all
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not think this is needed, the fetch-depth or any history to build for the e2e tests only.

Comment on lines +15 to +17
fetch-depth: 0
- name: Fetch all branches and tags
run: git fetch --all
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not think this is needed, the fetch-depth is a left over of the old pre-split flow and we should not need it here for the code coverage we run.

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (779450c) 69.52% compared to head (13cfd67) 69.46%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
- Coverage   69.52%   69.46%   -0.07%     
==========================================
  Files          50       50              
  Lines        7993     7993              
==========================================
- Hits         5557     5552       -5     
- Misses       2248     2252       +4     
- Partials      188      189       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants