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 #733

Closed
wants to merge 1 commit into from

Conversation

doupache
Copy link
Contributor

@doupache doupache commented Nov 27, 2023

What is this PR for?

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.

I encountered this issue when I manually triggered a GitHub Action in the k8shim fork repository. In contrast, manually triggering GitHub Actions in the core repository does not lead to this issue. This is because, in the core repository, the Golang CI is only executed when submitting a Pull Request.

However, I believe ensuring consistency in the make lint process between k8shim and core is the correct approach.

Perhaps we could consider aligning k8shim's GolangCI behavior with that of the core? (only triggering during PR)
cc @craigcondit @wilfred-s

What type of PR is it?

  • - Improvement

What is the Jira issue?

YUNIKORN-2098

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3b9c966) 77.83% compared to head (19b5d39) 77.83%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #733   +/-   ##
=======================================
  Coverage   77.83%   77.83%           
=======================================
  Files          80       80           
  Lines       13385    13385           
=======================================
  Hits        10418    10418           
  Misses       2646     2646           
  Partials      321      321           

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

@doupache
Copy link
Contributor Author

doupache commented Nov 27, 2023

related to k8shim #715

Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

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

  1. split the workflow for the push to master into its own file (like the k8shim)
  2. use the same naming convention as in the k8shim for the files
  3. remove the if 'pull_request' checks from the pre-commit steps

We really want all the checks to run when you kick off the manual flow as it catches a lot of small things.

@wilfred-s
Copy link
Contributor

With the changes made to clean up all lint issues we now run without SHA detection.
This jira is no longer relevant

@wilfred-s wilfred-s closed this Aug 14, 2024
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.

2 participants