fix: add project config validation when using args, fixes #5445 #5452
Conversation
|
Download the artifacts for this pull request:
See Testing a PR |
stasadev
force-pushed
the
20231024_stasadev_validate_project_config
branch
2 times, most recently
from
9578fc0
to
58856b8
Compare
rfay
force-pushed
the
20231024_stasadev_validate_project_config
branch
from
58856b8
to
b496f7e
Compare
|
Rebased |
There was a problem hiding this comment.
I'm confused about returning nil on error there, but it does function correctly in manual testing.
The one important thing to do is to improve the message. Currently we see something like Failed to get project(s): unsupported PHP version: xx.1, DDEV only supports the following versions: [5.6 7.0 7.1 7.2 7.3 7.4 8.0 8.1 8.2 8.3] but we need to identify the project that failed, so `Failed to load project 'projectname': ...
| @@ -462,6 +461,11 @@ func (app *DdevApp) ValidateConfig() error { | |||
| return err | |||
| } | |||
|
|
|||
| // Stop here if no config found | |||
| if err := CheckForMissingProjectFiles(app); err != nil { | |||
| return nil | |||
There was a problem hiding this comment.
Shouldn't we be returning the error here?
There was a problem hiding this comment.
Added more comments on this check.
stasadev
force-pushed
the
20231024_stasadev_validate_project_config
branch
from
b496f7e
to
a25f6a0
Compare
| @@ -462,6 +461,11 @@ func (app *DdevApp) ValidateConfig() error { | |||
| return err | |||
| } | |||
|
|
|||
| // Stop here if no config found | |||
| if err := CheckForMissingProjectFiles(app); err != nil { | |||
| return nil | |||
There was a problem hiding this comment.
Added more comments on this check.
| @@ -46,7 +46,7 @@ func getRequestedProjects(names []string, all bool) ([]*ddevapp.DdevApp, error) | |||
| if requestedProjectsMap[name], exists = allProjectMap[name]; !exists { | |||
| p := globalconfig.GetProject(name) | |||
| if p != nil && p.AppRoot != "" { | |||
| requestedProjectsMap[name] = &ddevapp.DdevApp{Name: name} | |||
| requestedProjectsMap[name] = &ddevapp.DdevApp{Name: name, AppRoot: p.AppRoot} | |||
There was a problem hiding this comment.
When you run ddev start <project> for a project with missing files, the error does not include the path:
Failed to start test-project: ddev can no longer find your project files at . If you would like...
And now it does:
Failed to start test-project: ddev can no longer find your project files at /home/stas/code/ddev/test-project. If you would like...
rfay
force-pushed
the
20231024_stasadev_validate_project_config
branch
from
124c355
to
a105ef9
Compare
|
Rebased for first chance to use the new testing stuff. Reviewing. |
There was a problem hiding this comment.
This looks great to me. Lots of improvements to user communication. 💯
One weird thing I encountered was on ddev start -a with a broken config on one of the projects (ddev.com), t:ype: xxx. It happily tried to start ddev.com (and didn't report errors) even though the config was invalid. I imagine that ddev start -a doesn't validate config. We don't have to fix that here.
Yes, there seems to be no check with the This should be easy to fix with: --- a/cmd/ddev/cmd/utils.go
+++ b/cmd/ddev/cmd/utils.go
@@ -27,6 +27,13 @@ func getRequestedProjects(names []string, all bool) ([]*ddevapp.DdevApp, error)
// If all projects are requested, return here
if all {
+ for _, project := range allProjects {
+ err = project.ValidateConfig()
+ if err != nil {
+ return nil, err
+ }
+ }
+
return allProjects, nil
}I can add this fix in a new PR, or here. |
|
Let's save it for a new PR, which can either go into this week's release or not, so we don't hold this one up. You can pull this one when you want. The orbstack results will be interesting if you have time to wait for them, but I imagine we'll have things to learn anyway. |
The Issue
ddev start project-name#5445How This PR Solves The Issue
Runs validation check for every projects that you pass to any DDEV command that uses args, so fixes more than just
ddev start.Manual Testing Instructions
See the issue.
Automated Testing Overview
Related Issue Link(s)
Release/Deployment Notes