Skip to content
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

bug fix: activate connection from callback _connection_updated_cb #3669

Merged
merged 1 commit into from Nov 9, 2021
Merged

Conversation

qyng
Copy link
Contributor

@qyng qyng commented Oct 28, 2021

Bug Fix: activate connection from callback _connection_updated_cb

this is the correct order
ui.tui.spokes.network: updating connection ...
ui.tui.spokes.network: updated connection ...
ui.tui.spokes.network: activating connection ...

otherwise it could be wrong as
ui.tui.spokes.network: updating connection ...
ui.tui.spokes.network: activating connection ...
ui.tui.spokes.network: updated connection ...

@poncovka
Copy link
Contributor

@rvykydal, could you review this pull request, please?

@jkonecny12 jkonecny12 added the master Please, use the `f39` label instead. label Nov 1, 2021
@rvykydal rvykydal added manual testing required This issue can't be merged without manual testing and removed manual testing required This issue can't be merged without manual testing labels Nov 2, 2021
Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for the fix!

I wonder if we should pass nm_client instance to the ConfigureDeviceSpoke rather then getting one in the callback but I don't have strong opinion on that.

@qyng
Copy link
Contributor Author

qyng commented Nov 3, 2021

@rvykydal, Passing the nm_client instance was also my first thinking in the beginning. But later I decided to get it on the fly to keep this fix small and interfaces clean, as the nm_client is already being well cached for us from the network module. We may overhaul this part when we have more things to pass between the two spokes.

@rvykydal
Copy link
Contributor

rvykydal commented Nov 4, 2021

/kickstart-test --testtype smoke

@rvykydal
Copy link
Contributor

rvykydal commented Nov 8, 2021

@qyng please remove the added merge commits from the branch.
I'll merge your PR then.

this is the correct order
   ui.tui.spokes.network: updating connection ...
   ui.tui.spokes.network: updated connection ...
   ui.tui.spokes.network: activating connection ...

otherwise it could be wrong as
   ui.tui.spokes.network: updating connection ...
   ui.tui.spokes.network: activating connection ...
   ui.tui.spokes.network: updated connection ...
@rvykydal
Copy link
Contributor

rvykydal commented Nov 9, 2021

/kickstart-test --testtype smoke

@rvykydal rvykydal merged commit 10a3318 into rhinstaller:master Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master Please, use the `f39` label instead.
4 participants