-
Notifications
You must be signed in to change notification settings - Fork 960
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
Use Variables for simplify edits and enhance readability. #4313
Use Variables for simplify edits and enhance readability. #4313
Conversation
991c95d
to
cc2bbe2
Compare
@Syspretor, if valid Ill work on number 2 ( Phony Targets ) |
@omerap12 Thank you very much for this PR. The classification and organization of variable definitions is great, but there are some areas where we hope to improve. First of all, ensuring that all modules in the Makefile execute successfully is a priority in the optimization work for the Makefile. During my testing, I found some typos and syntax issues that broke this, such as the variable definition for DOCKER_PLATFORM on line 28, and a typo regarding the -f flag on line 231, among similar issues. So please try running the commands included in the Makefile to ensure they still work smoothly. |
Got it. Ill test it myself then. Thanks for your review :) |
Signed-off-by: Omer Aplatony <[email protected]>
cc2bbe2
to
35aec70
Compare
Signed-off-by: trafalgarzzz <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4313 +/- ##
==========================================
- Coverage 62.90% 58.33% -4.58%
==========================================
Files 483 563 +80
Lines 28570 31277 +2707
==========================================
+ Hits 17972 18245 +273
- Misses 8393 10785 +2392
- Partials 2205 2247 +42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: trafalgarzzz <[email protected]>
Signed-off-by: trafalgarzzz <[email protected]>
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@omerap12 I made some minor changes to make Github Actions work with the latest Makefile. The PR LGTM, thanks for making it much more clear.
/lgtm
No worries, I’m planning to enhance it further :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang, TrafalgarZZZ The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Ⅰ. Describe what this PR does
Ⅱ. Does this pull request fix one issue?
Part of #4297
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews