Apply package name constraints to env vars, dirs #364
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Applies the constraints articulated in compose-spec/compose-spec#314 for project names from `COMPOSE_PROJECT_NAME`, the project dir, or the working dir. Before this change, the constraints applied only when using `docker compose -p` or the `name:` config file property. Non-normalized project names from `COMPOSE_PROJECT_NAME`, the project dir, or the working dir were still allowed, which `docker compose` would then normalize. The constraints are enforced by: - `cli.ProjectFromOptions()` setting `withNamePrecedenceLoad()` as an option to `loader.Load()`. - `loader.Load()` applies `withNamePrecedenceLoad()`, which tries to get the project name from the command line options, then `COMPOSE_PROJECT_NAME`, then the project or working dir. - `withNamePrecedenceLoad()` calls `loader.Options.SetProjectName()`, which now also saves the original name as well as the normalized name. - `loader.Load()` then calls `loader.projectName()`, which reads the project name from the config file(s) if it's not already set. - `loader.projectName()` then applies the new `CheckOriginalProjectNameIsNormalized()` function extracted from `cli.WithName()`. Only a handful of test cases required updating in `cli/options_test.go`. These test cases were checking that invalid project names were rejected by `cli.WithName()`, but that `cli.ProjectFromOptions()` would normalize them. Now `cli.ProjectFromOptions()` rejects them with the same error. There's no need for new test cases specifically for `COMPOSE_PROJECT_NAME` or directory-based project names, because the `CheckOriginalProjectNameIsNormalized()` function is applied regardless of the source. Other existing test cases are validating that `cli.ProjectFromOptions` does also use `COMPOSE_PROJECT_NAME` or directory names when necessary. Closes compose-spec#363. Signed-off-by: Mike Bland <[email protected]>
|
I see one of the Windows assertions failed. I can tend to this by some time Monday. Please also see the additional comment I left: #363 (comment) |
After applying package name constraints uniformly in the previous commit, cli/options_windows_test broke on Windows with: ``` --- FAIL: TestConvertWithEnvVar (0.00s) options_windows_test.go:35: assertion failed: error is not nil: "\\" is not a valid project name... ``` This is because the `TestConvertWithEnvVar` test was setting the working directory as `C://`. No other mechanism defined the package name, causing the non-normalized package name to be `/` and the normalized package name to be ``. Technically, this is exactly what we want: The root directory _shouldn't_ be used for the project name, which will be empty. This violates the new `^[a-z0-9][a-z0-9_-]*$` package name constraint. As a result, the solution is to update the working directory to `C:/working-dir/` and to adjust the expected results accordingly. Signed-off-by: Mike Bland <[email protected]>
Since the spec now explicitly rejects empty project names, it's worth explicitly testing for this case. Since the root directory will normalize as the empty string, it's worth specifying this requirement explicitly in a test case as well. (This is why the options_window_test started failing two commits ago, though that wasn't the intent of that test.) added an extra check to loader.CheckOriginalProjectNameIsNormalized specifically for empty project names. Signed-off-by: Mike Bland <[email protected]>
Turned out the assertion that the project name not be empty broke many other things. Adding the project name to a bunch of test data fixed it. Also, it turned out that Project.Name was never getting JSON marshaled. That's fixed in this commit, too. Signed-off-by: Mike Bland <[email protected]>
Fixes options_windows_test.go. As explained in the code comment, this leaves the prefix off of the error message, since it will differ between Windows and other systems. Signed-off-by: Mike Bland <[email protected]>
ndeloof
approved these changes
Mar 14, 2023
|
@ndeloof I appreciate the approval. Anything else needed before merging? (No rush, just following up.) |
laurazard
approved these changes
Mar 19, 2023
mbland
added a commit
to mbland/docs
that referenced
this pull request
Mar 19, 2023
These changes reflect updated `docker compose` behavior regarding acceptable project names. compose-spec/compose-go#261 changed `docker compose` behavior to require normalized project names as input when using `-p`. Previously `docker compose` normalized project names automatically, leading to errors for some users after the change landed in compose-spec/compose-go v1.2.5 and docker/compose v2.5.1. compose-spec/compose-spec#314 updated the compose spec, effectively enforcing the same constraint for the `name:` config file property. compose-spec/compose-go#362 added information to the error message specifying the correct project name format. compose-spec/compose-go#364 added enforcement of the new project name requirements for all project name mechanisms (`-p`, `name:`, `COMPOSE_PROJECT_NAME`, project dir, working dir). Local development URLs: - http://localhost:4000/compose/reference/#use--p-to-specify-a-project-name - http://localhost:4000/compose/environment-variables/envvars/#compose_project_name - http://localhost:4000/engine/reference/commandline/compose/#use--p-to-specify-a-project-name Production URLs: - https://docs.docker.com/compose/reference/#use--p-to-specify-a-project-name - https://docs.docker.com/compose/environment-variables/envvars/#compose_project_name - https://docs.docker.com/engine/reference/commandline/compose/#use--p-to-specify-a-project-name Signed-off-by: Mike Bland <[email protected]>
aevesdocker
pushed a commit
to docker/docs
that referenced
this pull request
Mar 22, 2023
* Add detailed project name requirements, mechanisms These changes reflect updated `docker compose` behavior regarding acceptable project names. compose-spec/compose-go#261 changed `docker compose` behavior to require normalized project names as input when using `-p`. Previously `docker compose` normalized project names automatically, leading to errors for some users after the change landed in compose-spec/compose-go v1.2.5 and docker/compose v2.5.1. compose-spec/compose-spec#314 updated the compose spec, effectively enforcing the same constraint for the `name:` config file property. compose-spec/compose-go#362 added information to the error message specifying the correct project name format. compose-spec/compose-go#364 added enforcement of the new project name requirements for all project name mechanisms (`-p`, `name:`, `COMPOSE_PROJECT_NAME`, project dir, working dir). Local development URLs: - http://localhost:4000/compose/reference/#use--p-to-specify-a-project-name - http://localhost:4000/compose/environment-variables/envvars/#compose_project_name - http://localhost:4000/engine/reference/commandline/compose/#use--p-to-specify-a-project-name Production URLs: - https://docs.docker.com/compose/reference/#use--p-to-specify-a-project-name - https://docs.docker.com/compose/environment-variables/envvars/#compose_project_name - https://docs.docker.com/engine/reference/commandline/compose/#use--p-to-specify-a-project-name Signed-off-by: Mike Bland <[email protected]> * Revert _data/compose-cli/docker_compose.yaml This reverts part of commit 9ce29a8. As @glours noted in the review of #16915, this content comes from the upstream docker/compose repo. He's opened docker/compose#10390 to apply the update there instead. Signed-off-by: Mike Bland <[email protected]> --------- Signed-off-by: Mike Bland <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Applies the constraints articulated in compose-spec/compose-spec#314 for project names from
COMPOSE_PROJECT_NAME, the project dir, or the working dir.Before this change, the constraints applied only when using
docker compose -por thename:config file property. Non-normalized project names fromCOMPOSE_PROJECT_NAME, the project dir, or the working dir were still allowed, whichdocker composewould then normalize.The constraints are enforced by:
cli.ProjectFromOptions()settingwithNamePrecedenceLoad()as an option toloader.Load().loader.Load()applieswithNamePrecedenceLoad(), which tries to get the project name from the command line options, thenCOMPOSE_PROJECT_NAME, then the project or working dir.withNamePrecedenceLoad()callsloader.Options.SetProjectName(), which now also saves the original name as well as the normalized name.loader.Load()then callsloader.projectName(), which reads the project name from the config file(s) if it's not already set.loader.projectName()then applies the newCheckOriginalProjectNameIsNormalized()function extracted fromcli.WithName().Only a handful of test cases required updating in
cli/options_test.go. These test cases were checking that invalid project names were rejected bycli.WithName(), but thatcli.ProjectFromOptions()would normalize them. Nowcli.ProjectFromOptions()rejects them with the same error.There's no need for new test cases specifically for
COMPOSE_PROJECT_NAMEor directory-based project names, because theCheckOriginalProjectNameIsNormalized()function is applied regardless of the source. Other existing test cases are validating thatcli.ProjectFromOptionsdoes also useCOMPOSE_PROJECT_NAMEor directory names when necessary.Closes #363.