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

vendor: update to the latest version of ferny #19555

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

allisonkarlitskaya
Copy link
Member

This has some breaking API changes, so make adjustments as necessary.

@@ -66,9 +73,6 @@ async def do_connect_transport(self) -> None:
transport = await self.spawn(self.args, env, stderr=agent, start_new_session=True)

if '# cockpit-bridge' in self.args:
Copy link
Member

Choose a reason for hiding this comment

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

I suppose splitting this code into two parts was done due to some changed ordering requirements? Although it's not clear to me why writing the askpass helper above would depend on an already initialized InteractionAgent -- in fact, the old ordering made more sense to me, as the agent surely depends on a working askpass script?

This ordering change could use at least some justification in the commit message.

Testing for this magic # cockpit-bridge string twice is a DRY. Not a strong veto, but it may be nicer to set a boolean flag for this, if the code split really is necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because the .add_handler() call on the interaction agent disappeared. All handlers have to be set at the start now, which necessitates the earlier creation of the handler.

I have an idea for how this can be done more elegantly.

This has some breaking API changes, so make adjustments as necessary.

One slightly non-obvious change comes from the removal of
InteractionAgent.add_handlers(), which had the potential to be racy in
case a message arrived before its handler was registered.  We now have
to specify all handlers up-front, which means that the block for doing
that needs to move up to before the transport is connected.  Of course,
we can only write the stage1 bootloader after the transport is online,
so we need to keep that in its current location.
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup!

@martinpitt
Copy link
Member

testSudo failure is an old acquaintance, but this unexpected message is entirely novel and exciting. Retrying, and on my todo list.

@martinpitt martinpitt merged commit b18df61 into cockpit-project:main Nov 9, 2023
93 checks passed
@martinpitt martinpitt deleted the new-ferny branch November 9, 2023 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants