-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix flb_utils test in docker environments #11098
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,8 @@ | |
| ] | ||
| } | ||
| }, | ||
|
|
||
| "mounts": [ | ||
| "source=/etc/machine-id,target=/etc/machine-id,ro,type=bind,consistency=cached" | ||
| ], | ||
|
Comment on lines
+36
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify devcontainer mounts behavior when The new
The fix looks sound for the target environment (WSL/devcontainer). Run this verification to confirm the mount syntax is correct per the devcontainer specification: 🏁 Script executed: #!/bin/bash
# Description: Verify the mounts syntax in devcontainer.json against the spec.
# Check if the JSON is valid
jq empty .devcontainer/devcontainer.json && echo "JSON is valid"
# Extract and display the mounts section for manual inspection
jq '.mounts' .devcontainer/devcontainer.jsonLength of output: 113 🏁 Script executed: #!/bin/bash
# Display the first ~10 lines to identify the parse error at line 5, col 4
echo "=== First 15 lines of devcontainer.json ==="
head -15 .devcontainer/devcontainer.json
echo -e "\n=== Lines 30-45 (mounts section) ==="
sed -n '30,45p' .devcontainer/devcontainer.json
echo -e "\n=== Full file with line numbers ==="
cat -n .devcontainer/devcontainer.jsonLength of output: 2340 🌐 Web query: 💡 Result: Short answer
Notes
Sources 🌐 Web query: 💡 Result: Short answer: Yes — devcontainer.json supports comments, but via JSONC (JSON with comments), not necessarily full JSON5 syntax. Use // or /* */ comments and keep the file as JSONC; if you run a linter, set its file type to JSONC (not strict JSON). [1][2][3] Sources:
💡 Result: Short answer
Sources
Docker bind mount will fail container startup if The mount syntax is valid per devcontainer specification, using correct Docker This breaks the devcontainer for systems without
🤖 Prompt for AI Agents
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is an important issue to resolve @dietWall There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @patrick-stephens I am looking right now how it could be solved in other ways, as I couldn´t find something like a conditional mount in devcontainers documentation. I can think of multiple different solutions, like setup function for this test, postCreateCommand in devcontainer or change the test. But I have to implement it to see if its working.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we had added some unit tests around this in the Calyptia days to allow you to specify machine ID, etc. as well explicitly - using /etc/machine-id is not recommended for lots of reasons not just that it may not be there :)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was my PR I was thinking of: #9691 Maybe instead of mounting things in it would be better to support an override we can then set for the container.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. After taking a quick look at PR #9691, I think I can reuse at least the test setup functions here. Probably even more, I need to carefully go through this changes. Right now I like the idea to generate a new machine-id if it not exists and reuse it later. If it exists, no generation is required. I would submit a new PR in the next days, as it changes the concept quite a lot. So we can close this one.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, relying on machine-id is a bad thing in general as you've seen :)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll let you close to make it clear there is a new approach |
||
| "remoteUser": "fluent-bit" | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new
mountsentry assumes/etc/machine-idexists on the host, but many contributors use Docker Desktop on macOS or Windows where that file is absent. In those environmentsdocker runwill refuse to start the dev container because the bind source does not exist, rendering the default devcontainer unusable. This solves the WSL case but regresses other platforms; consider making the mount optional or creating the file when present instead of requiring it globally.Useful? React with 👍 / 👎.