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
Validate config before writing .yaml or starting project, resolves #1095 #1139
Validate config before writing .yaml or starting project, resolves #1095 #1139
Conversation
5e5948c
to
131ada5
Compare
1b4bb58
to
40ddc90
Compare
The file check being removed breaks the use of the method as config validation before writing the config file, and when reading a config file to create a ddevapp struct, this check is performed _after_ the config file has been read.
6352c0d
to
3456f8c
Compare
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.
This is awesome in so many different ways, wow.
- Long-deferred need to use typed errors
- validation within the code
- Validation in config flags
- Validation in config.yaml
Way, way more ambitious than I was expecting, and so much better.
There's one switch that could be more compact, and maybe we can talk about inverting the logic in that very fragile code. But definitely approved.
$ ddev config --php-version=9.2
You are reconfiguring the project at /Users/rfay/workspace/d8git.
The existing configuration will be updated and replaced.
Found a drupal8 codebase at /Users/rfay/workspace/d8git
failed to validate config: invalid PHP version: 9.2, must be one of [5.6 7.0 7.1 7.2]
rfay@rfay-mbp-2017:~/workspace/d8git$ vi .ddev/config.yaml
rfay@rfay-mbp-2017:~/workspace/d8git$ ddev start
Unable to get project(s): invalid PHP version: 9.2, must be one of [7.1 7.2 5.6 7.0]
rfay@rfay-mbp-2017:~/workspace/d8git$ vi .ddev/config.yaml
rfay@rfay-mbp-2017:~/workspace/d8git$ ddev config --webserver-type=nothing
You are reconfiguring the project at /Users/rfay/workspace/d8git.
The existing configuration will be updated and replaced.
Found a drupal8 codebase at /Users/rfay/workspace/d8git
failed to validate config: invalid webserver type: nothing, must be one of [apache-fpm apache-cgi nginx-fpm]
return fmt.Errorf("no valid project config.yaml was found at %s", app.ConfigPath) | ||
provider, err := app.GetProvider() | ||
if err != nil { | ||
return err.(invalidProvider) |
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.
I love typing errors. That's going to be a big step forward for us. Thanks!
err = app.Init(activeAppRoot) | ||
if err != nil && (strings.Contains(err.Error(), "is not a valid hostname") || strings.Contains(err.Error(), "is not a valid apptype") || strings.Contains(err.Error(), "config.yaml exists but cannot be read.") || strings.Contains(err.Error(), "a project (web container) in ")) { | ||
return app, err | ||
if err = app.Init(activeAppRoot); err != nil { |
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.
This is a massive improvement, great with the error typing.
It will read a lot better with case webContainerExists, invalidHostname, invalidAppType, ... : return app, err
This is absolutely suspect code though, quite fragile, and as you'd expect it has a lot of history. I'd rather we had the types of error we're going to ignore here instead of the types we're going to return. That would be less fragile. We could do that either this time or next time.
|
||
// ValidProviders should be updated whenever provider plugins are added or removed, and should | ||
// be used to ensure user-supplied values are valid. | ||
var ValidProviders = map[string]bool{ |
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.
You coming over to the dark side of maps?
The Problem/Issue/Bug:
Only certain config values were being validated before attempting to start a project, which could lead to cryptic errors. When supplying config value flags to
ddev config
, no validation was done before writing the config file.How this PR Solves The Problem:
These changes do more to ensure that configuration values are valid before either 1) writing a config file, or 2) attempting to start a project. Additionally, many string values in the source code were constantized and a few helper functions were added as a requirement for valid value lookup.
Similarly, instead of inspecting error values, a small set of error types has been added allowing for error type comparisons, decoupling the error from its text value.
Manual Testing Instructions:
Using the CLI's config flags, ensure that invalid choices lead to validation errors:
--projectname invalid!name
--php-version 1.9
--projecttype typo4
--webserver-type nginx-cgi
--additional-hostnames valid,in_valid
--additional-fqdns valid.com,in_valid.com
Provide combinations of these invalid values alongside valid values to ensure ddev rejects invalid values and, when appropriate, outputs the valid values for that flag.
Using config.yaml, ensure that invalid values lead to validation errors after saving the file and when attempting to
ddev start
the project:name: "invalid name"
type: drupal10
php-version: 9.9
webserver-type: nginx-cgi
additional-hostnames: [valid, in_valid]
additional-fqdns: [valid.com, in_valid.com]
provider: unknown
Automated Testing Overview:
Constantization updates will largely rely on the build process completing successfully, ensuring all new
symbols are resolved correctly.
Test cases have been added to ensure invalid config values generated appropriate errors.
Related Issue Link(s):
#1095
Release/Deployment notes: