Allow use of "http://" prefix in HTTPS Proxy config #139
Conversation
|
@dsbibby, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
@dsbibby, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
1 similar comment
|
@dsbibby, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
@dsbibby, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
Apologies - I've tried to sign my commits to this PR as described but my github email privacy settings have messed things up I think. I've fixed that now but I'm not sure how resolve commit 29128ea |
|
@dsbibby one way is to rebase and amend the commit that is not signed. https://www.digitalocean.com/community/tutorials/how-to-rebase-and-update-a-pull-request |
|
@dsbibby, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
@pksrc, thank you - I'm still getting to grips with git! I think I've succeeded - dco-required label is removed at least an both my commits rebased in to one now. |
|
@dsbibby, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
@dsbibby You can ensure your commits are always signed by using |
|
@lamw - good tip, thank you! 🙂 |
|
@lamw - I think this is good for review again now. I'm not sure if the error handling is the best way approach? It certainly works, but is easily missed. |
|
@dsbibby Once you've addressed the latest changes, feel free to squash all commits into one and we should be good to merge. |
dsbibby
force-pushed
the
patch-1
branch
2 times, most recently
from
f1eb4ee
to
a810b4f
Compare
Updated OVF properties to suit previous commit Signed-off-by: David Bibby <17053612+dsbibby@users.noreply.github.com> Changed logic to require "http(s)://" on proxy URLs rather than make it optional. Signed-off-by: David Bibby <davidsbibby@gmail.com> Cleaned up formatting Signed-off-by: David Bibby <davidsbibby@gmail.com> Fixed logic around error handling of malformed proxy URLs Signed-off-by: David Bibby <davidsbibby@gmail.com> Fixed proxy error messages not appearing on console output Signed-off-by: David Bibby <davidsbibby@gmail.com> Improved readability with better variable naming Signed-off-by: David Bibby <davidsbibby@gmail.com>
|
@lamw - all sensible suggestions - I renamed the |
|
@dsbibby The updates looks good, thanks! One final question, did you build the appliance to make sure all changes worked as expected? We normally use this a final confirmation as part of the PR. |
|
@lamw I hadn't (I'd just tested in place in an exisitng appliance) but building properly is the correct thing to do, and sounded fun, so I have now! All seems to work as expected. I've deployed in to an environment that needs proxies and one that doesn't and both work as expected. Also, passing a malformed URI as a proxy field logs an error during first boot of the appliance. |
|
@dsbibby We just realized this PR was against |
|
@lamw Yes, I still have the branch. Go ahead and revert and I'll submit a new PR when you're ready. |
|
@dsbibby Thanks. We've reverted, go ahead and submit a new PR against development branch and we'll get that merged up. |
If a http(s):// prefix is specified for either HTTP or HTTPS Proxy OVF properties, then use that rather than assuming either.