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-2760] make tools should check the version of tools #880

Closed
wants to merge 12 commits into from

Conversation

blueBlue0102
Copy link
Contributor

@blueBlue0102 blueBlue0102 commented Jul 18, 2024

What is this PR for?

Makefile, by default, checks only the existence of file. Hence, developers need to remove tools folder (or call make distclean) manually to trigger the installation after we update the version of tools.

However, how developers can be aware of the tools updates? Personally, I smell fishy from the error of warning, but that could be implicit and noisy :cry

In order to fix that, I'd like to introduce the new folder structure to tools folder:

/tools/{tool_name}-{version}

That offers a unique path to each version of tool. Developers will not miss the updates anymore.

What type of PR is it?

  • - Improvement

What is the Jira issue?

YUNIKORN-2760

How should this be tested?

Currently, make tools is triggered only by ./scripts/run-e2e-tests.sh
To verify whether make tools checks the tools' versions, call any command in ./scripts/run-e2e-tests.sh or trigger it manually

Screenshots (if appropriate)

image

@blueBlue0102 blueBlue0102 marked this pull request as draft July 19, 2024 14:45
Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.06%. Comparing base (f281908) to head (1a28948).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #880      +/-   ##
==========================================
+ Coverage   67.30%   68.06%   +0.75%     
==========================================
  Files          70       70              
  Lines        7625     7624       -1     
==========================================
+ Hits         5132     5189      +57     
+ Misses       2274     2217      -57     
+ Partials      219      218       -1     

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

@pbacsko
Copy link
Contributor

pbacsko commented Jul 23, 2024

@blueBlue0102 please check the e2e test output, something is wrong with the "ginkgo" command, I suppose it is related to your change.

@pbacsko pbacsko self-requested a review July 25, 2024 17:22
@blueBlue0102
Copy link
Contributor Author

@pbacsko The problem has been solved, and the e2e test has passed on my forked repo. Please let me know if there are any further issues. Thank you!

@blueBlue0102 blueBlue0102 marked this pull request as ready for review July 27, 2024 12:56
Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

+1 LGTM

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@chia7712
Copy link
Contributor

@blueBlue0102 thanks for this great improvement!

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.

4 participants