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

Resolve shellcheck lint issues #100

Merged

Conversation

epiccurious
Copy link
Collaborator

No description provided.

@epiccurious epiccurious marked this pull request as draft March 24, 2024 23:59
@epiccurious
Copy link
Collaborator Author

This refactor will require testing. Keeping in Draft for now.

@epiccurious
Copy link
Collaborator Author

@BenWestgate please see my responses to your questions here and here on my fork.

@epiccurious epiccurious changed the title Refactor to resolve shellcheck lint issues Resolve shellcheck lint issues Mar 25, 2024
@epiccurious
Copy link
Collaborator Author

Resolves #95.

@epiccurious
Copy link
Collaborator Author

Here are the four ShellCheck rules that are excluded in the GitHub Actions lint:

@BenWestgate BenWestgate marked this pull request as ready for review March 25, 2024 03:56
BenWestgate
BenWestgate previously approved these changes Mar 25, 2024
Copy link
Owner

@BenWestgate BenWestgate left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Waiting on a VM test to merge.

@BenWestgate BenWestgate marked this pull request as draft March 25, 2024 04:05
@epiccurious epiccurious marked this pull request as ready for review March 28, 2024 19:11
@epiccurious epiccurious marked this pull request as draft March 28, 2024 19:15
Copy link
Owner

@BenWestgate BenWestgate left a comment

Choose a reason for hiding this comment

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

Need to test this in VM. Code looks good to me.

@epiccurious epiccurious marked this pull request as ready for review May 3, 2024 10:53
@epiccurious
Copy link
Collaborator Author

epiccurious commented May 3, 2024

@BenWestgate To make it easier to do testing, can you please enable this setting at bottom of the main Settings page?

Screenshot

@epiccurious epiccurious force-pushed the 95-resolve-shellcheck-lint-issues branch from 2303324 to 0391c05 Compare May 3, 2024 11:05
@epiccurious
Copy link
Collaborator Author

Rebased.

@epiccurious
Copy link
Collaborator Author

I'm seeing an issue with the PR during testing. Something is broken with the clone/download process. Will post a screen recording showing the failure.

@epiccurious epiccurious marked this pull request as draft May 3, 2024 11:52
@epiccurious
Copy link
Collaborator Author

Here's the video recording for the failure:
Screencast from 2024-05-03 11-35-22.webm

Here's the video recording for Bails master branch succeeding:
Screencast from 2024-05-03 11-47-02.webm

PR isn't ready to merge yet unfortunately.

@BenWestgate
Copy link
Owner

@BenWestgate To make it easier to do testing, can you please enable this setting at bottom of the main Settings page?

Screenshot

It already is:
image

@BenWestgate
Copy link
Owner

BenWestgate commented May 7, 2024

Here's the video recording for the failure: Screencast from 2024-05-03 11-35-22.webm

Here's the video recording for Bails master branch succeeding: Screencast from 2024-05-03 11-47-02.webm

PR isn't ready to merge yet unfortunately.

So the download for the signatures threw an error, you can see that in the terminal and that's the Zenity message as well.

Did you look up what that error meant?

Did you try again starting fresh?

It may have been a once off download failure, this PR does not touch the code that downloads the signatures so Master is the same. Which means this error is non-blocking unless you always got it.

@epiccurious
Copy link
Collaborator Author

It may have been a once off download failure

You were right. I re-ran it again on the same branch and it's downloading fine. That's the first time I've seen that intermittent failure. Perhaps a wget command that could use a retry=5 argument and/or better error handling.

Setting PR as ready to review. Ready to merge.

@epiccurious epiccurious marked this pull request as ready for review May 9, 2024 10:12
@BenWestgate
Copy link
Owner

Perhaps a wget command that could use a retry=5 argument and/or better error handling.

The default is to retry 20 times, with the exception of fatal errors like “connection refused” or “not found” (404), which are not retried.

How would you suggest the error handling be improved? Just remove the option to try again? Since it already tried many times in order to fail there in the first place?

Copy link
Owner

@BenWestgate BenWestgate left a comment

Choose a reason for hiding this comment

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

Thank you for your PR!

@BenWestgate BenWestgate enabled auto-merge (rebase) May 9, 2024 11:56
@BenWestgate BenWestgate disabled auto-merge May 9, 2024 11:56
@BenWestgate BenWestgate merged commit b22f191 into BenWestgate:master May 9, 2024
2 checks passed
BenWestgate added a commit that referenced this pull request May 16, 2024
* fix: exclude double-quoted variables rule

* refactor: fix lint issues in persistent-setup

* refactor: increase indentation of onlynet_onion

* refactor: satisfy shellcheck SC2181

This refactor doesn't make a ton of sense so another option is to revert this commit and exclude SC2181.

* refactor: fix indentation of link_dotfiles subshell

* refactor: avoid for loops over find output [SC2044]

* refactor: comment out unused variable

* refactor: ensure variable never expands to /bin or /lib

* refactor: use -n instead of ! -z

* Ignore SC2046 in CI checks

* fix syntax in yaml

* refactor: read without -r will mangle backslashes

* refactor: Declare and assign separately to avoid masking return values

* refactor: read without -r will mangle backslashes

* refactor: Don't use variables in the printf format string

* refactor: $ is unnecessary on arithmetic variables

* Remove the repeated ', providing ... privacy.'

* Clarify that Bitcoin Core connects, not Bails

* Make consistent with Lines 113 and 156

* Exclude SC2012 (Use 'cd ... || exit') and SC2164 (find instead of ls)

* wording change in comment

* lint fixes

---------

Co-authored-by: Ben Westgate <[email protected]>
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.

2 participants