-
Notifications
You must be signed in to change notification settings - Fork 304
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
login1: add NewWithConnection and RebootWithContext methods #390
base: main
Are you sure you want to change the base?
login1: add NewWithConnection and RebootWithContext methods #390
Conversation
0722452
to
b3aa1e3
Compare
e187552
to
484c87e
Compare
1.12 is now EOL for almost 2 years: https://endoflife.date/go. As a reference, supported Ubuntu versions use either 1.10 or 1.13: https://packages.ubuntu.com/search?keywords=golang-go So I think this bump is reasonable. This is done to get access to stdlib methods like errors.Is and formatting statement %w, which is de facto standard for handling errors nowadays. Closes coreos#391 Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
This is a desired way of testing to avoid creating fragile test suites and be able to refactor code without touching tests. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
This method allows passing existing D-Bus connection, which allows to re-use connection between clients and to mock D-Bus connection for testing purposes. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
Existing Reboot() method does not allow using context not inspecting D-Bus call errors, which makes it difficult to debug and use. This commit adds new RebootWithContext() method which addresses those shortcomings. Closes coreos#387 Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
484c87e
to
1a186bc
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.
Thanks for the PR!
This is unfortunately becoming a kitchen sink, which isn't nice.
Let's only add RebootWithContext
in here, everything else will be touched elsewhere.
Hey, thanks for quick feedback. Would you like me to create one PR per commit from this PR then? Generally, I'd prefer to not submit code without tests, no matter how trivial it is, and changes here as the simplest steps to get things testable. Also, I'm concerned that "everything else" will not get enough attention once |
Generally speaking yes, I prefer handling separate PRs as they can easily travel at different speeds and merge at different times. Regarding testing, it usually goes in the same PR, agreed. In this case though it is both a large reworking and pretty much just formalized mocking. It effectively does not cover much juicy content, other than package-global constants and other static strings. |
This PR does some small improvements to
login1
package, namely changes tests to use blackbox approach to avoid building up the tech debt by adding new tests without doing so in the first place.Then adds
NewWithConnection
method together with tests as a new constructor to login1.Conn (#389), which allows re-using and mocking D-Bus connection so remaining method calls can be tested.Finally,
RebootWithContext
method is added as an replacement and improved version ofReboot
.Closes #387
Closes #391
Submitted code is based on the work I've done in flatcar/flatcar-linux-update-operator#112.