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

[ci] fix shellcheck warnings in CI scripts #6646

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

vnherdeiro
Copy link
Contributor

Fixing shellcheck warnings in .ci/test.sh .ci/test-r-package.sh .ci/setup.sh continuing !6641

@jameslamb jameslamb changed the title Fixing shellcheck in .ci/test.sh .ci/test-r-package.sh .ci/setup.sh [ci] fix shellcheck warnings in CI scripts Sep 15, 2024
Copy link
Collaborator

@jameslamb jameslamb left a 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.

.ci/setup.sh Outdated Show resolved Hide resolved
.ci/test-r-package.sh Outdated Show resolved Hide resolved
.ci/test-r-package.sh Outdated Show resolved Hide resolved
.ci/test.sh Outdated Show resolved Hide resolved
.ci/test.sh Outdated Show resolved Hide resolved
.ci/test.sh Outdated Show resolved Hide resolved
.ci/test.sh Outdated Show resolved Hide resolved
.ci/test.sh Outdated Show resolved Hide resolved
.ci/test.sh Show resolved Hide resolved
.ci/test-r-package.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@jameslamb jameslamb left a 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.

.ci/test-r-package.sh Outdated Show resolved Hide resolved
.ci/test-r-package.sh Outdated Show resolved Hide resolved
.ci/test-r-package.sh Outdated Show resolved Hide resolved
.ci/test-r-package.sh Outdated Show resolved Hide resolved
@jameslamb jameslamb self-requested a review September 16, 2024 16:25
Copy link
Collaborator

@jameslamb jameslamb left a 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.

.ci/test-r-package.sh Outdated Show resolved Hide resolved
.ci/test.sh Outdated Show resolved Hide resolved
.ci/test.sh Outdated Show resolved Hide resolved
@vnherdeiro
Copy link
Contributor Author

@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.

@jameslamb
Copy link
Collaborator

I am still struggling with the UX here, I don't understand why some of my comments are not visible if not logged in for instance.

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.

@jameslamb
Copy link
Collaborator

let me know of any further comment.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants