Do not de-register, and stop rich-logger on jmp run#658
Do not de-register, and stop rich-logger on jmp run#658mangelajo wants to merge 1 commit intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
12-19: Make "run" detection robust and avoid installing rich traceback when not using RichHandler.Relying on sys.argv[1] breaks for nested subcommands (e.g., "jmp exporter run") or alternative invocation forms. Also, if we're deliberately avoiding RichHandler for "run", we should skip installing rich traceback there too to keep output fully plain.
Suggested refactor:
- traceback.install() - # there is no way to determine if the command is invoked for jmp run or something else at this - # point based on ctx and params, so we just look at sys.argv - if len(sys.argv) > 1 and sys.argv[1] == "run": - # on a exporter run we don't want to use RichHandler for logs, just plain logs for the system journal - basicConfig = partial(logging.basicConfig) - else: - basicConfig = partial(logging.basicConfig, handlers=[RichHandler()]) + # Determine if invoked as "... run" (supports nested subcommands) + cmd_path = getattr(ctx, "command_path", "") or "" + is_run = cmd_path.split()[-1:] == ["run"] + if not is_run: + # Fallback: scan argv for bare "run" tokens, ignoring flags + argv_cmds = [a for a in sys.argv[1:] if not a.startswith("-")] + is_run = "run" in argv_cmds + + # Only install rich traceback when using RichHandler + if not is_run: + traceback.install() + + if is_run: + # On exporter run we want plain logs for the system journal + basicConfig = partial(logging.basicConfig) + else: + basicConfig = partial(logging.basicConfig, handlers=[RichHandler()])Please confirm the actual command path(s) used (e.g., "jmp run" vs "jmp exporter run") so we can tailor the detection precisely. If needed, I can adjust the matcher to check for specific full paths (ctx.command_path).
packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
67-67: Include context in exit log for easier triage.Add lease and registration info to the exit line to aid journald searches.
- logger.info("Exporter is exiting") + logger.info("Exporter is exiting (lease_name=%s, registered=%s)", self.lease_name, self.registered)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/exporter.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
🔇 Additional comments (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
2-2: LGTM: importing sys for argv inspection.packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
62-66: Verify controller-side cleanup semanticsEnsure exporter registrations are dropped on disconnect/lease end; if server-side GC or TTL/heartbeat isn’t guaranteed, add a short TTL/heartbeat contract or reinstate explicit deregistration to avoid stale entries.
|
posted #659 as an alternative for unregistration |
As we talked about, your approach makes more sense, I will split the rich logger fix into its own PR |
Summary by CodeRabbit
New Features
Refactor
Chores