Skip to content

Conversation

@r2evans
Copy link
Contributor

@r2evans r2evans commented May 6, 2017

In a DD-WRT environment, I don't always have the ability to set the CA certificates for connecting to secure websites. The right answer is to add certs so that curl will verify correctly. A hack can be to allow --insecure, at least for testing. By setting CURL="curl --insecure" at the top of this patch (or an addition for --cacerts), it works as anticipated.

This patch does not require the "POSIX" patch but I'm using them combined for the purposes of running on my DD-WRT router.

unset QUIET

CURL="curl"

Copy link
Collaborator

Choose a reason for hiding this comment

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

when already defining a variable for curl, I would go for $(which curl) instead of a hardcoded curl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, thanks. I've changed to the previous behavior (of hard-coded curl) and now use $CURLOPTS ... it's easy enough to use curl=$(which curl), but that's up to the maintainer(s) and was not the intent of my PR. Pls see my recent commit.

Copy link
Owner

Choose a reason for hiding this comment

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

Since there's already a $(which curl) check in an if statement near the beginning of the script to verify that curl is available, using just "curl" should be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yes, forgot that.

@r2evans
Copy link
Contributor Author

r2evans commented May 12, 2017

@Red5d @fbartels, do you know what the intent is for maintaining pushbullet-bash? There has been one commit in the last year (in the readme), your last commit was a year ago, and there's not been a lot of other activity on it. (Likewise, not a lot of "issues" either, which could be a good sign.)

I'm not rushing or trying to take over, I just don't want to find out in six months that it's terminal on the back-burner with no intention for active development.

Thanks to @Red5d and you, btw: I like the tool and use it all the time (as my involvement suggest).

@Red5d
Copy link
Owner

Red5d commented May 13, 2017

I've been kinda busy the past few days, but I'm going to try and finish and merge your contributions shortly.

There hasn't been a lot of activity on this tool mostly because it hasn't been needed apart from bugfixes and some adjustments a while back to address api changes and implement newly-available features.

@fbartels , if you're interested, I'll add you as a collaborator on this repo so you can help review/merge pull requests. You already help out in the Issues and looking over code in pull requests (which is much appreciated).

@r2evans
Copy link
Contributor Author

r2evans commented May 13, 2017

Thanks for the prompt feedback. I had hoped the slow rate of change was due more to relative stability. (I'm also surprised that PB hasn't changed their API much in the last year.) Let me know what I can do.

@fbartels
Copy link
Collaborator

@Red5d hi, sorry I'm on vacation and only saw your comment now. I'll gladly take the higher repo permissions. Already have bookmarked these issues for some further testing once I'm back.

@Red5d
Copy link
Owner

Red5d commented May 23, 2017

@fbartels , great, thanks! I've added you and apparently there's a link that's been emailed to you which you'll have to click on to accept it.

@fbartels fbartels merged commit b1a5366 into Red5d:master May 28, 2017
@r2evans r2evans deleted the curlOptions branch May 29, 2017 15:57
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.

3 participants