Fixes #18936 - checking server against CAs in TFTP module #792
Conversation
|
Can one of the admins verify this patch? |
theforeman-bot
added
Waiting on contributor
Needs testing
Not yet reviewed
and removed
Waiting on contributor
labels
|
Hello Anna and welcome to the community! We require all commits to be in format of `Fixes #18936 - some short subject". We have a bot that enforces this. Can you please squash both commits into a single one with this commit message? |
|
Nice patch, thank you. I left a few nitpicks. There is an open question of the default value, let's discuss this with @ekohl and either keep it or change it. |
|
Certainly a nice addition @avitova. I'm in favor of verifying certificates whenever you can. I also thought about the default. I'm torn between compatibility (current implementation) and security. I'm leaning to merging this (after @lzap's comment have been addressed). Right now we're still not 100% certain if the next version will be 2.6 or 3.0 (to be decided in the next few days). If the next version is 3.0, then we can merge another patch that flips the default to be secure. |
|
I do not know why tests are started: [test] |
|
ok to test |
|
I think something went wrong in merging it. You should rebase, not merge develop into your branch. |
|
Yes, I tried to rebase but unfortunately was not able to and somehow broke it. I will discuss it with lzap soon and clean all the mess hopefully. |
avitova
force-pushed
the
avito
branch
3 times, most recently
from
af52cc8
to
e217d01
Compare
avitova
force-pushed
the
avito
branch
2 times, most recently
from
4529cbe
to
66b1190
Compare
ekohl
changed the title
ekohl
changed the title
There was a problem hiding this comment.
Code wise 👍 but I'd like to see a bit more descriptive commit message. I already added TFTP to the PR title. https://chris.beams.io/posts/git-commit/ has a lot more tips.
|
Oh yeah, if you could add "TFTP" into the commit subject that would be better. |
|
Thank you, next time I'll focus on the message bit more. |
|
For the record, I told Anna not to exceed 50 characters in commit subjects, but it's not a hard rule. If you exceed and the subject cannot be reasonably shortened, it is fine. Just do not waste space, do not use articles, just the simplest meaningful statement. |
|
For the record, I tested that wget accepts arguments at the very end after the URL: Works fine, please rebase the subject change and we are merging. Thanks. |
|
One thing I see a lot of people do wrong is not using the imperative mood (https://chris.beams.io/posts/git-commit/#imperative). So I'd write |
|
Can you remove the trailing dot? Read the guide that Ewoud linked above, these are unwanted :-) |
|
Sure, I actually did read it, but using the dot when starting with capital, is my habit, an unpleasant one in this case. I'm sorry, it should be fixed by now. |
|
I pushed while on the wrong branch, nothing has changed. Hopefully it will not be a problem. |
No description provided.