-
Notifications
You must be signed in to change notification settings - Fork 22
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
KUBESAW-129: Makefile target to check compatibility of local version with other repos #417
Conversation
… with other repos Signed-off-by: Feny Mehta <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #417 +/- ##
=======================================
Coverage 78.54% 78.54%
=======================================
Files 49 49
Lines 2023 2023
=======================================
Hits 1589 1589
Misses 376 376
Partials 58 58 |
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
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.
Looks good! I've tried it and it seems to be working now 👍
I was wondering if we should place this script in toolchain-cicd and download it here and in api at runtime, since the api pr contains similar/same code if I'm not wrong.
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.
Looks good overall, left a few comments.
Does it delete the tmp folder after running the command? if not, can we consider adding it?
Signed-off-by: Feny Mehta <[email protected]>
it is automatically removed/deleted once the system restarts.. i did consider adding a delete command.. but that becomes a bit risky, incase that folder is not found .. and hence wanted to avoid it and use this.. |
The code is almost same, the difference is in the replace command and api has one more repo toolchain common to run the verify on .. but thought about keeping them in their respective repos for now |
Ok makes sense for now to keep them both in their respective repos, and we can see how they evolve later on. Thanks 👍 |
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.
The code looks good and works fine on my machine 👍
I have a couple of questions:
-
should we place the script inside some folder , maybe create a
scripts
folder or a subfolder inmake/scripts/verify-replace.sh
if it's simpler to execute from there. -
what is the plan for adding this check to the GH CI, will that be done in follow up PRs ?
That can be done.
this same script should work when called from github actions.. i intend to test this for CI and add this to the CI for each new PR |
Signed-off-by: Feny Mehta <[email protected]>
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.
Thank you for addressing my comments, looks good to me
thanks for the clarification. Thus you are planning to update the GH CI job in this PR. |
I was thinking to take that as a separate PR. |
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.
Looks good 👍
Thanks for addressing my comments and answering my questions 🙏
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.
Looks good, a few comments
for e in ${ERRORLIST[*]} | ||
do | ||
echo "$e" | ||
done |
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.
we should exit with a non-zero value if something failed, right?
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.
not sure if we require the non zero exit here ?,
here we are just displaying the error repo names, and error list is already being checked if its empty or not , so my understanding was we dont require exit here.?
Probably i am missing something here 🤔
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.
if one of the verification failed, then we want the whole makefile target to return non-zero value (in other words let it fail as well)
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.
updated it
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[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.
Nice 👍
This second part of dependency check
There is a makefile target that validates compatibility of the local version in other repositories that rely on them
Similar-Related PR in API - codeready-toolchain/api#437