Fix: respect required: false for env_file in publish command#13652
Fix: respect required: false for env_file in publish command#13652maks2134 wants to merge 2 commits intodocker:mainfrom
Conversation
Fixes docker#13648 The docker compose publish command was ignoring required: false setting on env_file entries, causing failures when optional env files were missing. Changes made: - Modified checkForSensitiveData() to skip env files with required: false - Updated processFile() to only process required env files into layers - Fixed checkEnvironmentVariables() to only consider required env files - Added comprehensive tests to verify the fix Signed-off-by: Maks Kozlov <maks@example.com> Signed-off-by: maks2134 <maks210306@yandex.by>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
IMHO, we should allow non-required env files, check the sensitive data and add them as a layer when present on the user’s host.
| if !envFile.Required { | ||
| continue | ||
| } |
There was a problem hiding this comment.
If the file does exist on the user's host, it should still be published as a layer and skipped by default
| if !envFile.Required { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Same here, we should check env variables if the file exist on user's host
- Process optional env files if they exist on disk (instead of skipping all) - Improve error message to mention "required env_file" for clarity - Add test coverage for optional env files that exist - Fix linting issues Signed-off-by: Maks Kozlov <maks@example.com> Signed-off-by: maks2134 <maks210306@yandex.by>
|
Please take a look at the changes when you have the opportunity. @glours |
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
This is still not the right expect behaviour, we should check the env files content if the files exist even if there are not mandatory
| for _, envFile := range service.EnvFiles { | ||
| if envFile.Required { | ||
| errorList[service.Name] = append(errorList[service.Name], fmt.Sprintf("service %q has required env_file declared.", service.Name)) | ||
| break | ||
| } |
There was a problem hiding this comment.
We should also return an error when the file is not required but present on the user disk, because it will be upload as part of the Compose artifact
|
|
||
| err := service.checkEnvironmentVariables(project, api.PublishOptions{}) | ||
| assert.Assert(t, err != nil) | ||
| assert.Assert(t, strings.Contains(err.Error(), `service "test" has required env_file declared.`)) |
There was a problem hiding this comment.
nit
| assert.Assert(t, strings.Contains(err.Error(), `service "test" has required env_file declared.`)) | |
| assert.ErrorContains(t, err, `service "test" has required env_file declared.`) |
3 participants
What I did
Fixed docker compose publish command to respect
required: falsesetting on env_file entries. Previously, the command would fail when optional env files were missing, even when explicitly marked as not required.Changes made:
required: falseRelated issue
fixes #13648
(not mandatory) A picture of a cute animal, if possible in relation to what you did