refactor: Use project_list.yaml as source of truth, fixes #5918 #5926

GuySartorelli · 2024-03-04T07:12:54Z

The Issue

Users have been confused by having two copies of the project list, and haven't known which to update if they need to manually update the list.

How This PR Solves The Issue

If there's no project_list.yaml file and the global_config.yaml file contains a project list (i.e. if someone is upgrading from an older version of DDEV), copy the list into project_list.yaml
- This is a departure from the previous behaviour which would copy the list from global_config.yaml if there was any difference between the two lists
Remove ProjectList from the GlobalConfig type
- This means the next time global_config.yaml is written, the project list will be removed from that file.

Note that I have done this in two commits to show why some of the changes are necessary. I'll add PR comments to help clarify this.

Manual Testing Instructions

Ensure you have at least one project in your project list prior to checking out this PR
Remove project_list.yaml if you have it
Run ddev list - it should list the project(s) correctly, regardless of what state they were in
Confirm that you now have a project_list.yaml file
Run any command that will result in global config being written (e.g. if your last_started_version has changed, run ddev start in a project)
Confirm that global_config.yaml no longer has a list pf projects

Automated Testing Overview

No automated tests need to change - they were all already relying on project_list.yaml.

Release/Deployment Notes

This shouldn't affect anyone other than reducing confusion.

github-actions · 2024-03-04T07:17:10Z

Download the artifacts for this pull request:

See Testing a PR.

GuySartorelli · 2024-03-04T07:13:56Z

pkg/globalconfig/global_config.go

+	// Remove empty project list from global config
+	// In a future release it will be removed from the struct but
+	// for now it's needed to make upgrading easier.
+	cfgbytes = []byte(strings.Replace(string(cfgbytes), "project_info: {}", "", 1))


This is to ensure we don't have an empty project list in global config. It felt quite hacky, but was better than keeping an empty list in the file which would lead to further confusion.

GuySartorelli · 2024-03-04T07:14:49Z

pkg/globalconfig/global_config.go

-	ProjectList                      map[string]*ProjectInfo     `yaml:"project_info"`
+	// ProjectList is retained to make upgrading easier and should be removed after a few releases
+	ProjectList map[string]*ProjectInfo `yaml:"project_info"`


I didn't like keeping this in the type since it's no longer intended to be kept there, but this is how we'd do this if we don't have an intermediary legacy type.

GuySartorelli · 2024-03-04T07:16:24Z

pkg/globalconfig/global_config.go

+// Old versions of DDEV stored the project list in the global config.
+// For ease of upgrade, we need to be able to extract that data and move it to
+// the new project_list.yaml
+// In a future release, remove this.
+type LegacyGlobalConfig struct {
 	ProjectList map[string]*ProjectInfo `yaml:"project_info"`
 }


This lets us remove ProjectList from the GlobalConfig type. It means any time the global config is saved, the project list (if there was one in the file) is removed without having to do something hackish like an above comment points out.

It also gives us a clean way to marshal the list from the YAML file - if we removed ProjectList from GlobalConfig but didn't add something like this, we'd have to parse the file manually which would be even worse.

GuySartorelli · 2024-03-04T07:18:45Z

pkg/globalconfig/global_config.go

+	// Get the legacy project list if there is one
+	err = yaml.Unmarshal(source, &ddevLegacyGlobalConfig)
+	if err != nil {
+		return err
+	}


We do this in ReadGlobalConfig because this is the best point to guarantee that:

We haven't overwritten the config file yet

We haven't run ReadProjectList yet

GuySartorelli · 2024-03-04T07:19:53Z

pkg/globalconfig/global_config.go

+			// If someone upgrades from an earlier version, the global config may hold the project list.
+			if ddevLegacyGlobalConfig.ProjectList != nil && len(ddevLegacyGlobalConfig.ProjectList) > 0 {
+				DdevProjectList = ddevLegacyGlobalConfig.ProjectList
+				err := WriteProjectList(DdevProjectList)
+				// Whether there's an error or nil here we want to return
 				return err
 			}


We only want to check legacy config if the file doesn't exist. If the file exists, it's the source of truth even if it's empty.

GuySartorelli · 2024-03-04T07:25:19Z

pkg/globalconfig/global_config.go

+		// Whether there's an error or nil here we want to return
+		return err


We either created a new file here, or we had an unexpected error. Either way there's no projects to read so we can skip the "read the projects list from the file" logic below.

rfay · 2024-03-05T21:54:04Z

Moved to draft until tests start working

rfay · 2024-03-20T14:54:07Z

This is essential for DDEV v1.23.0 IMO, thanks for all your work on it, and we know you don't have unlimited time for it.

rfay · 2024-03-20T16:38:39Z

Rebased

GuySartorelli · 2024-03-21T21:36:30Z

Rebased again 'cause GitHub said it was still out of date with Master.

The error can be easily reproduced by running

GOTEST_SHORT=1 make test  TESTARGS="-run TestConfigDatabaseVersion"

    config_test.go:521: 
        	Error Trace:	/home/gsartorelli/dump/temp/ddev/cmd/ddev/cmd/config_test.go:521
        	Error:      	Received unexpected error:
        	            	exit status 1
        	Test:       	TestConfigDatabaseVersion
        	Messages:   	Failed to run ddev config --database mariadb:10.11: this project root /home/gsartorelli/tmp/ddevtest/TestConfigDatabaseVersion3259693668 already contains a project named TestConfigDatabaseVersion. You may want to remove the existing project with "ddev stop --unlist TestConfigDatabaseVersion"

If we keep this part of the code from ReadProjectList() (along with keeping ProjectList in the GlobalConfig struct), the tests pass:

if !reflect.DeepEqual(DdevGlobalConfig.ProjectList, DdevProjectList) {
	DdevProjectList = DdevGlobalConfig.ProjectList
	err := WriteProjectList(DdevProjectList)
	if err != nil {
		return err
	}
}

In other words, something in the tests is either writing directly to the project list in global config, or writing the wrong thing directly to the new project list. The two get out of sync, so it grabs everything from the global config and discards whatever's in the new list. Or maybe that's a red herring, I'm really not sure.

I think the problem might lie in RemoveProjectInfo somehow? Or maybe its in the logic of the TestConfigDatabaseVersion test itself? I'm not quite sure, and could use a hand tracing this.

GuySartorelli · 2024-03-21T21:51:56Z

I tried adding

err = globalconfig.RemoveProjectInfo(t.Name())
assert.NoError(err)

into the for _, dbTypeVersion := range versionsToTest { loop just after running app.stop() but that didn't work.

Appending strconv.Itoa(rand.Int()) after t.Name() for the project name in the ddev config command within the loop in the test also doesn't work. It still complains about an existing project named "TestConfigDatabaseVersion". It seems that it's not the name of the project being created that matters.

GuySartorelli · 2024-03-21T22:07:24Z

Changing the name of the first project that's created in the test changes the name of the project that is included in the error message. So it's clearly explicitly complaining that that first-created project exists and points to the same directory that the config call in the loop is trying to create a project in.

I just for the life of me don't seem to be able to find a way to remove that project correctly before creating the next one.

GuySartorelli · 2024-03-21T22:14:19Z

Okay, I have something that passes that test locally now.
Fingers crossed.

I'm pretty sure the change I made to the test is okay, but please do check that the test is still testing what it needs to test.

GuySartorelli · 2024-03-22T00:05:10Z

auth-ssh_test.go is failing, but only in one CI run... any guesses as to whether that's intermittent and would clear up if it was re-run? I'm guessing it probably would, since that seems unrelated to these changes?

rfay · 2024-03-22T02:47:41Z

I re-ran the nginx-fpm test. It's always good to understand what's going on with a fail, but you're always welcome to restart.

GuySartorelli · 2024-03-22T02:53:08Z

I only have the ability to restart the buildkite builds - I don't seem to have the right permissions for restart gha ones.

rfay · 2024-03-22T03:08:07Z

Sent you invitation. Hopefully that will give you the power.

GuySartorelli · 2024-03-22T03:19:09Z

Aha! Yes, thank you, I have the option now for next time 💙

GuySartorelli · 2024-03-22T04:41:56Z

All green! 🥳

tyler36 · 2024-03-22T04:51:31Z

Great work @GuySartorelli!

rfay

I'm sure this is fine, and will run through a quick test, but would love to discuss/consider just doing

	ProjectList                      map[string]*ProjectInfo     `yaml:"project_info,omitempty"`

and make sure that it gets emptied after processing.

rfay · 2024-03-24T20:01:32Z

cmd/ddev/cmd/config_test.go

@@ -514,6 +514,8 @@ func TestConfigDatabaseVersion(t *testing.T) {
 	// add a database into the config and nothing else
 	for _, dbTypeVersion := range versionsToTest {
 		_ = app.Stop(true, false)
+		_, err := exec.RunHostCommand(DdevBin, "delete", "-Oy", t.Name())


I'm not sure how this does any harm, but what did you hit here that made you do this? Is it related to this PR? Did some old project get left laying around?

See #5926 (comment) and all my comments in this PR since then :p

Could you please revisit this without the delete here to see if you can find out what the real problem is? It appears that you're having to patch something that is caused by something entirely different. This kind of adaptation of a test is really suspicious, as you know.

It looks to me like this code is just trying to call ddev config inside a directory which already has a ddev project created and that this should have been failing previously

Except... it doesn't seem to have failed except in this PR. Thanks!

Yup. The bit of code I mentioned in #5926 (comment) is being removed in this PR - if we keep that there we don't get the error, because there's no projects in the global config list and there are projects in the new projects list. That discrepancy combined with the mentioned code results in clearing the new project list, so as far as DDEV is concerned there's no project there.

This PR removes that code, which means DDEV is now fully aware that there is a project there, causing the error.

I'm not sure if this was something introduced when I added the new project list file, or since then, or maybe I'm just wrong about the cause here? But that's what it looks like to me.

Thanks for chasing it. I'm often unable to follow your references, too many context switches. If you could re-mention things instead of linking to other comments maybe I'll do better. It sounds like you're saying that another test has left a project around instead of doing an app.Stop(true, false) or something.

Not another test - this same test. It's running ddev config in a loop in the same directory. It is doing app.Stop(true, false) but that seems to be insufficient to remove the project from the project list file.

If app.Stop(true,false) isn't removing the project from the project list file, it's an introduced bug. Every single test would fail if that were common. Please step through in debugger and figure out what's going on. Thanks!

Good to know - I'll introduce a new test that explicitly tests app.Stop(true,false) and debug it. If such a test already existed I wouldn't have come up with this solution since it would have been clear what I found was a red herring.
I'll try to find time this week to do this.

rfay · 2024-03-24T20:02:36Z

cmd/ddev/cmd/config_test.go

@@ -514,6 +514,8 @@ func TestConfigDatabaseVersion(t *testing.T) {
 	// add a database into the config and nothing else
 	for _, dbTypeVersion := range versionsToTest {
 		_ = app.Stop(true, false)
+		_, err := exec.RunHostCommand(DdevBin, "delete", "-Oy", t.Name())
+		assert.NoError(err)


I've been leaning more toward require() lately, which stops the test. Less time chasing irrelevant things. It's not necessary to accept this, I'm just commenting.

Suggested change

assert.NoError(err)

require.NoError(t, err)

Might be worth making a push to change all of them at some stage and get CI to catch that.
I'll change this if you want me to use the omitempty discussed in all the other comments - but if you're happy leaving that as is then I'll leave this as well.

It's not important. You're just such an important contributor that I'm doing a bit of coaching. Nothing wrong with what's here.

rfay · 2024-03-24T20:03:47Z

pkg/globalconfig/global_config.go

@@ -33,6 +32,8 @@ var (
 	DdevGlobalConfig GlobalConfig
 	// DdevProjectList is the list of all existing DDEV projects
 	DdevProjectList map[string]*ProjectInfo
+	// Old versions of DDEV stored the project list in the global config.
+	ddevLegacyGlobalConfig LegacyGlobalConfig


Interesting technique, but couldn't we just allow the old ProjectList in current config, but not write it out?

I assume that's what the omitempty does in #5926 (review)?

If so, I have no problem doing that. I just wasn't aware that was an option.
When I tried doing it without omitempty it ended up with an empty map in the yaml file which is obviously misleading - for anyone who looks at that file it'd look like DDEV has incorrectly said there's no projects!

rfay · 2024-03-24T20:05:08Z

pkg/globalconfig/global_config.go

+// the new project_list.yaml
+// In a future release, remove this.
+type LegacyGlobalConfig struct {
+	ProjectList map[string]*ProjectInfo `yaml:"project_info"`


As above, tempted to allow the ProjectList in current global config, just not write it (just make sure it's empty and omitempty

rfay · 2024-03-24T20:14:29Z

I did manually test and look through, and it works perfectly as is.

GuySartorelli · 2024-03-24T20:15:37Z

Let me know if you want the omitempty thing or not - it's unclear from your comments whether it's fine as is or whether that's blocking merging.

rfay · 2024-03-24T21:02:05Z

Let me know if you want the omitempty thing or not

I would really appreciate it if you could experiment with that approach. I think it might be cleaner.

GuySartorelli · 2024-03-24T21:04:26Z

Let me know if you want the omitempty thing or not

I would really appreciate it if you could experiment with that approach. I think it might be cleaner.

Sweet, I'll try make some time this week to get that done.

rfay · 2024-03-28T17:09:16Z

Rebased, fixed merge conflicts.

rfay · 2024-04-01T13:48:36Z

@stasadev or I will experiment with the omitempty if you don't have time for it and that's OK with you. Want to get this in this week. Thanks!

GuySartorelli · 2024-04-01T20:09:22Z

Sweet, yes please do.

stasadev · 2024-04-02T20:05:40Z

I think the problem might lie in RemoveProjectInfo somehow?

@GuySartorelli, your idea was in the right direction, the project list was not updated, when it was needed.

That's because we had globalconfig.WriteGlobalConfig() inside app.Stop() - this statement is responsible for updating the project list.

I rewrote RemoveProjectInfo and removed globalconfig.WriteGlobalConfig() from app.Stop() - this is not needed anymore.

I'm sure this is fine, and will run through a quick test, but would love to discuss/consider just doing omitempty

Done, running tests.

rfay

I reviewed this again and manually tested, and it looks great to me! Thanks so much! 🙏🏼

GuySartorelli requested a review from a team as a code owner March 4, 2024 07:12

github-actions bot added the maintenance label Mar 4, 2024

GuySartorelli commented Mar 4, 2024

View reviewed changes

GuySartorelli mentioned this pull request Mar 4, 2024

Move to use project_list.yaml exclusively for project list #5918

Closed

GuySartorelli commented Mar 4, 2024

View reviewed changes

rfay marked this pull request as draft March 5, 2024 21:53

rfay force-pushed the 20240304_GuySartorelli_project-list branch from d3676e3 to ac05207 Compare March 20, 2024 16:38

GuySartorelli force-pushed the 20240304_GuySartorelli_project-list branch from ac05207 to 7e3f55e Compare March 21, 2024 21:05

GuySartorelli force-pushed the 20240304_GuySartorelli_project-list branch from 7e3f55e to e34353a Compare March 21, 2024 22:14

GuySartorelli marked this pull request as ready for review March 22, 2024 04:41

GuySartorelli requested a review from rfay March 24, 2024 20:06

rfay requested changes Mar 24, 2024

View reviewed changes

rfay requested a review from stasadev March 25, 2024 14:11

rfay force-pushed the 20240304_GuySartorelli_project-list branch from e34353a to 9785509 Compare March 28, 2024 17:09

GuySartorelli and others added 7 commits April 2, 2024 20:14

refactor: Use project_list.yaml as source of truth

b190466

refactor: Remove project list from global config entirely.

eb426b3

refactor Resolve linting issue

5ffa40a

refactor fix unit tests

06e0fd3

Update docs and schema

40c7593

Use omitempty for ProjectList

cdd1883

fix: always update project list

46aafe9

stasadev force-pushed the 20240304_GuySartorelli_project-list branch from 9785509 to 46aafe9 Compare April 2, 2024 19:54

GuySartorelli requested a review from a team as a code owner April 2, 2024 19:54

stasadev approved these changes Apr 3, 2024

View reviewed changes

stasadev requested a review from rfay April 3, 2024 11:09

rfay approved these changes Apr 3, 2024

View reviewed changes

rfay merged commit bdae26f into ddev:master Apr 3, 2024
23 checks passed

GuySartorelli deleted the 20240304_GuySartorelli_project-list branch April 3, 2024 20:04

		// Whether there's an error or nil here we want to return
		return err

refactor: Use project_list.yaml as source of truth, fixes #5918 #5926

refactor: Use project_list.yaml as source of truth, fixes #5918 #5926

Conversation

GuySartorelli commented Mar 4, 2024

The Issue

How This PR Solves The Issue

Manual Testing Instructions

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

github-actions bot commented Mar 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Mar 4, 2024 • edited

Choose a reason for hiding this comment

rfay commented Mar 5, 2024

rfay commented Mar 20, 2024

rfay commented Mar 20, 2024

GuySartorelli commented Mar 21, 2024 • edited

GuySartorelli commented Mar 21, 2024 • edited

GuySartorelli commented Mar 21, 2024

GuySartorelli commented Mar 21, 2024 • edited

GuySartorelli commented Mar 22, 2024 • edited

rfay commented Mar 22, 2024

GuySartorelli commented Mar 22, 2024

rfay commented Mar 22, 2024

GuySartorelli commented Mar 22, 2024

GuySartorelli commented Mar 22, 2024

tyler36 commented Mar 22, 2024

rfay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Mar 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Mar 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Mar 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfay commented Mar 24, 2024

GuySartorelli commented Mar 24, 2024

rfay commented Mar 24, 2024

GuySartorelli commented Mar 24, 2024

rfay commented Mar 28, 2024

rfay commented Apr 1, 2024

GuySartorelli commented Apr 1, 2024

stasadev commented Apr 2, 2024

rfay left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 4, 2024 •

edited

GuySartorelli Mar 4, 2024 •

edited

GuySartorelli commented Mar 21, 2024 •

edited

GuySartorelli commented Mar 21, 2024 •

edited

GuySartorelli commented Mar 21, 2024 •

edited

GuySartorelli commented Mar 22, 2024 •

edited

GuySartorelli Mar 24, 2024 •

edited

GuySartorelli Mar 24, 2024 •

edited

GuySartorelli Mar 24, 2024 •

edited