pybridge: Implement cockpit-bridge --version; some small cleanups #19234
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martinpitt
commented
Aug 24, 2023
| @@ -270,6 +271,9 @@ def main(*, beipack: bool = False) -> None: | |||
| if args.packages: | |||
| Packages().show() | |||
| return | |||
| elif args.version: | |||
| print(f'Version: {__version__}\nProtocol: 1') | |||
There was a problem hiding this comment.
The C bridge looks like this:
Version: 287
Protocol: 1
Payloads: dbus-json3 http-stream1 http-stream2 stream packet fsread1
fsreplace1 fswatch1 fslist1 null echo websocket-stream1
Authorization: crypt1
The "Authorization: crypt1" is hardcoded in the C bridge, and thus not really useful. We could implement "Payloads:" if we care, but I doubt it's actually useful.
There was a problem hiding this comment.
For some reason the inconsistency of how we get the two newlines printed out is bothering me. I will ignore that little nag, though. :)
martinpitt
force-pushed
the
pybridge-version
branch
3 times, most recently
from
2362493
to
09f5d87
Compare
|
Meh, this isn't worth the renaming, I backed out that commit from this PR. |
martinpitt
force-pushed
the
pybridge-version
branch
from
09f5d87
to
499d1ec
Compare
martinpitt
requested review from
allisonkarlitskaya
and removed request for
allisonkarlitskaya
allisonkarlitskaya
requested changes
Aug 31, 2023
| @@ -270,6 +271,9 @@ def main(*, beipack: bool = False) -> None: | |||
| if args.packages: | |||
| Packages().show() | |||
| return | |||
| elif args.version: | |||
| print(f'Version: {__version__}\nProtocol: 1') | |||
There was a problem hiding this comment.
For some reason the inconsistency of how we get the two newlines printed out is bothering me. I will ignore that little nag, though. :)
martinpitt
force-pushed
the
pybridge-version
branch
from
499d1ec
to
14dff0d
Compare
src/cockpit/bridge.py
Outdated
| @@ -62,12 +62,12 @@ def export(self, path: str, obj: bus.BaseObject) -> None: | |||
| class Bridge(Router, PackagesListener): | |||
| internal_bus: InternalBus | |||
| packages: Optional[Packages] | |||
| bridge_rules: List[JsonObject] | |||
| bridge_configs: List[BridgeConfig] | |||
There was a problem hiding this comment.
The commit message for this part is no longer accurate, since this was never a JsonObject...
This previously initialized `self.bridge_rules`, but never actually updated it, due to the `bridge_configs` vs. `bridge_rules` confusion. Change both the class member and the local variable to `bridge_configs` to become consistent with `Packages.get_bridge_configs()`. Fix the type of this rules list. mypy spotted this after fixing the variable confusion.
It has never been in the manpage, it isn't implemented (it just starts the protocol), and the C version's output is next to useless for users. We can implement this if/when we ever get an use case.
It was defined in argparse, but entirely ignored, so that it started the protocol. Also add it to the manpage. This also unbreaks the regular cockpit/ws container builds, as they use --version to define the tag. Add an integration test which exercises all non-protocol options. Fixes cockpit-project#19233
martinpitt
force-pushed
the
pybridge-version
branch
from
14dff0d
to
842a7db
Compare
| args: argparse.Namespace | ||
|
|
||
| def __init__(self, args: argparse.Namespace): | ||
| self.internal_bus = InternalBus(EXPORTS) | ||
| self.bridge_rules = [] | ||
| self.bridge_configs = [] |
There was a problem hiding this comment.
I guess () would be nicer here, but no biggie.
allisonkarlitskaya
approved these changes
Aug 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It was defined in argparse, but entirely ignored, so that it started the
protocol. Also add it to the manpage.
This also unbreaks the regular cockpit/ws container builds, as they use
--version to define the tag.
Add an integration test which exercises all non-protocol options.
Fixes #19233
See https://quay.io/repository/cockpit/ws?tab=tags , this stopped updating since we moved Fedora 38 to the pybridge.