fix(publish): honor env_file required: false for missing files#13848
Conversation
glours
left a comment
There was a problem hiding this comment.
Thanks for picking this up and aligning with the #13652 review, the fix is in the right shape. Two small asks before merging:
- Dead branch in
processFile(publish.go:262-264): theif envFile.Requiredpath is unreachable in production because the project loader already enforcesrequired:at load time. Suggest dropping it (and its test) or commenting it as defensive only. - Path leak on optional missing (
publish.go:265): when the file is absent we skipReplaceEnvFile, so the published YAML keeps the local path instead of the<hash>.envplaceholder. Since the hash is computed from the path string alone, we can still rewrite it — keeps the artifact filesystem-agnostic and consistent with the file-present case.
| if envFile.Required { | ||
| return nil, fmt.Errorf("env file %s not found", envFile.Path) | ||
| } |
There was a problem hiding this comment.
Nit: this if envFile.Required branch is unreachable here, the project loader (called from runPublish without SkipResolveEnvironment) already rejects missing required env files via compose-go's loadEnvFile.
The test only reaches it by forcing SkipResolveEnvironment: true.
Could you either drop the branch (and Test_processFile_required_env_file_missing) or add a comment marking it as belt-and-suspenders?
The guard in checkForSensitiveData is fine, that one is reachable because loadUnresolvedFile does set SkipResolveEnvironment.
There was a problem hiding this comment.
Dropped them, thanks!
| if envFile.Required { | ||
| return nil, fmt.Errorf("env file %s not found", envFile.Path) | ||
| } | ||
| continue |
There was a problem hiding this comment.
Could you still call transform.ReplaceEnvFile with the hash in this skip branch? The hash is derived from envFile.Path only (not file contents), so it's deterministic whether the file exists or not.
Without the rewrite, the published YAML keeps the publisher's local (resolved-to-absolute) path instead of the opaque <hash>.env placeholder, small filesystem-layout leak, and inconsistent with the file-present case.
There was a problem hiding this comment.
Good catch and thanks! Done. path no longer leaks now :)
Signed-off-by: Ijtihed Kilani <ijtihedk@gmail.com>
56e642c to
be3a4c7
Compare
glours
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution 🙏
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
What I did
When env_file declares required false and the file is missing then docker compose publish now skips the file instead of failing. If the optional file exists it is still scanned for sensitive data and included as a layer which preserves existing behavior.
Two locations needed guards:
Non-ENOENT errors (permissions, etc.) still propagate regardless of the required flag.
Related issue
Fixes #13648
Supersedes #13650, #13651, #13652 with a diff implementing what the maintainers request in #13652 review
(unrelated photo of my fiance's cute cat :3)