-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
networking: Add support for WireGuard #19024
Conversation
Oops, I should've written the new tests before pushing. The bot is not pleased at all. |
Rewrote tests using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial early feedback. This is going well, thanks!
This comment was marked as outdated.
This comment was marked as outdated.
301cee2
to
1ea465d
Compare
Rebased. Renamed the test file and class to something proper. And introduced install_dialog if the |
This comment was marked as duplicate.
This comment was marked as duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This is looking great! I have some feedback about Martin's feedback and my own feedback based on the screenshot.
These elements are connected. It should probably be an input group with a text input and a button. (See design docs.) The button label should say "Generate"; the form element already has a label of "Private key".
No. This is not a password. It also needs to also be copyable. Do not make it a password entry and don't add a reveal button. Additional feedback:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more small issues, but the bulk of TODOs are still in the previous rounds of open threads. Thanks!
This comment was marked as resolved.
This comment was marked as resolved.
f2155e7
to
332f4bf
Compare
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I resolved all previous review threads, and for easier tracking I also moved all of mine and most of Garrett's UI review into threads. I'm happy to help with the two last generic "create vs. edit" issues in a separate PR, unless you want to do it yourself.
aa0bc6c
to
a02131c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
(Yesterday evening and this morning, I've been working on mockups and sharing in Matrix; they're not here yet. I'll upload soon.) |
69e0670
to
3f01ea5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
ba5df71
to
b7a0e7a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
59c1e9a
to
c20234c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
3991a87
to
e49ec19
Compare
@garrett 's designs from above are implemented correctly (with the minor changes discussed afterwards). The ? icon position got fixed. I also extensively tested valiation/error messages with wrong values, pasting key, etc. This works really well now! 👏 This needs at least a pixel ref update, and we should also introduce a new pixel test for the dialog, as it has some non-trivial layout and e.g. the question mark position. I'll also go through the code once more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Just some trivialities, I'll deal with them together with the pixel test updates.
e49ec19
to
1f53987
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the pixel test, added a new one for the dialog, fixed the two trivial issues above, and updated the release note to current screenshots. Let's land! once green! 🚀
@subhoghoshX congratulations! Can you please look at the two follow-ups in the description?
model, | ||
modify, | ||
{ | ||
fail_text: cockpit.format(_("Creating this $0 will break the connection to the server, and will make the administration UI unavailable."), type == 'vlan' ? 'VLAN' : type), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
rollback_on_failure: type != 'vlan', | ||
}); | ||
} catch (e) { | ||
setDialogError(typeof e === 'string' ? e : e.message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
I have the remaining test failures on my radar, they are unrelated. |
TODO:
Follow-ups:
Networking: Add support for WireGuard
The Networking page can now create and edit WireGuard VPN connections.
Many thanks to Subho Ghosh for adding this feature as part of his Google Summer of Code project! and thanks to Gil Obradors for his initial work.