Improve privileged mode error message with actionable suggestions#448
Improve privileged mode error message with actionable suggestions#448danielschlegel wants to merge 3 commits intomainfrom
Conversation
danielschlegel
requested review from
kmannislands
and removed request for
a team
|
| Branch | Total Count |
|---|---|
| main | 5577 |
| This PR | 5589 |
| Difference | +12 (0.22%) |
📁 Changes by file type:
| File Type | Change |
|---|---|
| Go files (.go) | ❌ +8 |
| Documentation (.md) | ➖ No change |
| Earthfiles | ❌ +4 |
Keep up the great work migrating from Earthly to Earthbuild! 🚀
💡 Tips for finding more occurrences
Run locally to see detailed breakdown:
./.github/scripts/count-earthly.shNote that the goal is not to reach 0.
There is anticipated to be at least some occurences of earthly in the source code due to backwards compatibility with config files and language constructs.
danielschlegel
force-pushed
the
fix-issue-326-suggest-privileged-mode
branch
from
ffc8803 to
282f9d6
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the error messaging when a build requires privileged mode but the -P flag is missing. It introduces more descriptive user messages, including target information when available, and provides clear instructions on how to resolve the issue. Additionally, it moves the help message printing to the logbus formatter and adds comprehensive integration tests. Feedback includes addressing potential duplicate output caused by the architectural change in the formatter, fixing a grammatical error in the user message, and removing a redundant flag check before calling VerboseWarnf.
danielschlegel
force-pushed
the
fix-issue-326-suggest-privileged-mode
branch
4 times, most recently
from
1c5e841 to
9729fdd
Compare
Replace cryptic 'unlazy force execution: failed to load LLB: security.insecure
is not allowed' error with clear, user-friendly message.
Changes:
- cmd/earthly/app/run.go: Create clear error message with 3 solution options
* Run with -P flag
* Set EARTHLY_ALLOW_PRIVILEGED environment variable
* Add to config file
* Technical details only shown with -V or --debug flags
- logbus/formatter/formatter.go: Print help message for build failures
- tests/with-docker/Earthfile: Add simple test target for privileged tests
- tests/Earthfile: Add comprehensive tests for all privileged operations
* RUN --privileged
* WITH DOCKER
* IF --privileged
* FOR --privileged
Before:
Error: build target: build main: failed to solve: unlazy force execution:
failed to load LLB: security.insecure is not allowed
Help: earth --allow-privileged (earth -P) flag is required
After:
Error: This build uses privileged operations which requires privileged mode.
Help: To fix this, use one of the following:
• Run with the -P flag: earth -P +your-target
• Set environment variable: export EARTHLY_ALLOW_PRIVILEGED=true
• Add to config: earth config global.allow_privileged true
Clean up error handling: remove duplicate HelpPrint calls and simplify verbose warning
danielschlegel
force-pushed
the
fix-issue-326-suggest-privileged-mode
branch
from
9729fdd to
5ebf72a
Compare
| } | ||
|
|
||
| helpMsg := "To fix this, use one of the following:\n" + | ||
| " • Run with the -P flag: earth -P +your-target\n" + |
There was a problem hiding this comment.
Can we infer the target at this point? If the user executed earth +foobar, we can provide meaningful feedback ... earth -P +foobar....
There was a problem hiding this comment.
…f placeholder - Extract target from command line arguments when not available from interpreter error - Show specific command like 'earth -P +foobar' instead of generic 'earth -P +your-target' - Add comprehensive test suite for target extraction logic - Addresses feedback from @janishorsts to provide more meaningful error messages Fixes the issue where users had to mentally substitute the target placeholder with their actual target name in privileged mode error suggestions.
| } | ||
|
|
||
| // If no target info from interpreter error, try to extract from args | ||
| if targetInfo == "" && len(args) > 1 { |
There was a problem hiding this comment.
Hmm this is CLI args? We might not want this fallback.
Interpreter should capture this in the error it throws since we very likely have target a executing b which calls c and so on.
Repeating the top level target the user called which may not be where -P is required is not very helpful
3 participants
Replace cryptic 'unlazy force execution: failed to load LLB: security.insecure is not allowed' error with clear, user-friendly message.
Changes:
cmd/earthly/app/run.go: Create clear error message with 3 solution options
logbus/formatter/formatter.go: Print help message for build failures
tests/with-docker/Earthfile: Add simple test target for privileged tests
tests/Earthfile: Add comprehensive tests for all privileged operations
Before:
Error: build target: build main: failed to solve: unlazy force execution:
failed to load LLB: security.insecure is not allowed
Help: earth --allow-privileged (earth -P) flag is required
After:
Error: This build uses privileged operations which requires privileged mode.
Help: To fix this, use one of the following:
• Run with the -P flag: earthly -P +your-target • Set environment variable: export EARTHLY_ALLOW_PRIVILEGED=true • Add to config: earthly config global.allow_privileged true
Fixes #326