-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[ci] fix shellcheck warnings in CI scripts #6646
base: master
Are you sure you want to change the base?
[ci] fix shellcheck warnings in CI scripts #6646
Conversation
82334f0
to
36d08e2
Compare
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.
Thanks for all your effort on this!
Similar to #6641, I've left some inline requests for you to please describe the exact shellcheck
warnings that these changes are intended to address.
I've also left a lot of in-review suggested code changes. Please accept all of those by clicking "add to batch" and "commit suggestion(s)" in the browser, not by pushing your own new commits, so all of the threads will auto-resolve. If you're not familiar with how that works on GitHub, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request#applying-suggested-changes.
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[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.
Thanks for your continued effort on this. Please see the next round of suggestions I left. I'll review again once you've addressed those and some of the other comments from my previous review that haven't yet been addressed.
Co-authored-by: James Lamb <[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.
I just left another set of comments. Please also go through all the not-yet-resolved prior comment threads and address the suggestions / questions there.
Co-authored-by: James Lamb <[email protected]>
@jameslamb I think I have adressed all comments, I am still stuggling with the UX here, I don't uderstand why some of my coments are not visible if not logged in for instance. Anyways, let me know of any further comment. |
I don't know, but strongly recommend just logging in whenever you use GitHub. You'll need to be logged in to leave comments anyway. |
Thanks, there's nothing else for you to do for now. I've pushed some commits implementing the other things I would have asked for in the next round of comments... I want to get these PRs moving. I'll come back and review one more time once we've unblocked CI (#6654) and will ask other maintainers to review at that time (since this now contains some code I wrote too). |
…ightGBM into shellcheck_ci_files_changes
Fixing shellcheck warnings in .ci/test.sh .ci/test-r-package.sh .ci/setup.sh continuing !6641