Assorted fixes#319
Conversation
Reviewer's GuideLowered verbosity for startup logs, standardized flag formatting in the gvproxy script, and corrected the default RESTful URI description in the usage documentation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cfergeau - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| VM_NAME="$(basename ${DISK_IMAGE})" | ||
|
|
||
| ${GVPROXY} -mtu 1500 -ssh-port 2223 -listen-vfkit unixgram://$(pwd)/${VM_NAME}.sock -log-file ${VM_NAME}.gvproxy.log --pid-file ${VM_NAME}.gvproxy.pid & | ||
| ${GVPROXY} -mtu 1500 -ssh-port 2223 -listen-vfkit unixgram://$(pwd)/${VM_NAME}.sock -log-file ${VM_NAME}.gvproxy.log -pid-file ${VM_NAME}.gvproxy.pid & |
There was a problem hiding this comment.
I would use -- for all commands since it is more consistent with other commands in this file, and most commands, but this is good enough.
They are dumping some internal data structures using %+v which is not very user friendly. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
By default it’s set to `none://`: ``` pkg/cmdline/cmdline.go:const DefaultRestfulURI = "none://" ``` Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lstocchi, nirs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThe changes update logging levels in the Go code, switch script command-line options to long-form flags, and clarify documentation regarding the default state of a RESTful service option. No control flow, logic, or public interfaces were altered. Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/vfkit/main.go (1)
75-77: Downgraded logs are good, but the blankInfo()still emits noiseSwitching the two messages to
Debugfis 👍.
However the subsequentlog.Info()(with no arguments) still writes an info-level empty line, defeating the goal of keeping the regular log output quiet.- log.Info() + log.Debug() // keep the visual separator, but at debug levelThis keeps the visual separation without re-introducing info-level chatter.
doc/usage.md (1)
18-19: Minor wording / spacing cleanup-The URI (address) of the RESTful service. By default it’s disabled. Valid schemes are +The URI (address) of the RESTful service. By default, it is disabled. Valid schemes areRemoves the double spaces, swaps the curly apostrophe for ASCII, and adds a comma for readability.
contrib/scripts/start-gvproxy.sh (1)
26-26: Quote path expansions to survive spaces and satisfy ShellCheck
$(pwd)or${VM_NAME}can contain spaces; unquoted they will be split into
multiple words (SC2046). Quoting all expansions also makes the command more
robust.-${GVPROXY} --mtu 1500 --ssh-port 2223 --listen-vfkit unixgram://$(pwd)/${VM_NAME}.sock --log-file ${VM_NAME}.gvproxy.log --pid-file ${VM_NAME}.gvproxy.pid & +"${GVPROXY}" --mtu 1500 \ + --ssh-port 2223 \ + --listen-vfkit "unixgram://$(pwd)/${VM_NAME}.sock" \ + --log-file "${VM_NAME}.gvproxy.log" \ + --pid-file "${VM_NAME}.gvproxy.pid" &(You may also want to quote the
cp,trap, andvfkitinvocations for full
consistency.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/vfkit/main.go(1 hunks)contrib/scripts/start-gvproxy.sh(1 hunks)doc/usage.md(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
contrib/scripts/start-gvproxy.sh
[warning] 26-26: Quote this to prevent word splitting.
(SC2046)
🪛 LanguageTool
doc/usage.md
[grammar] ~18-~18: Use proper spacing conventions.
Context: ...e URI (address) of the RESTful service. By default it’s disabled. Valid schemes...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~18-~18: Use proper spacing conventions.
Context: ...ful service. By default it’s disabled. Valid schemes are tcp, none, or `uni...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~18-~18: There might be a mistake here.
Context: ...efault it’s disabled. Valid schemes are tcp, none, or unix. In the case o...
(QB_NEW_EN_OTHER)
Follow-up to #319
3 participants
One doc fix reported in libkrun/krunkit#51 (comment)
One consistency fix reported in #317 (comment)
And silencing of 2 debug-looking logs.
Summary by Sourcery
Silence nonessential logs, standardize script flag naming, and improve documentation clarity for the RESTful service default
Enhancements:
Documentation:
Summary by CodeRabbit
Documentation
Style
Refactor