Skip to content
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

refactor: move projects list into its own yaml file, fixes #5639 #5651

Merged

Conversation

GuySartorelli
Copy link
Collaborator

@GuySartorelli GuySartorelli commented Dec 19, 2023

The Issue

How This PR Solves The Issue

Moves the projects list into its own ~/.ddev/project_list.yaml file.

For now this is a fairly naive approach - I'd probably lean towards calling the file ~/.ddev/project_list.yaml, and renaming all of the GlobalProject* that I've added to ProjectList, i.e.

  • GetGlobalProjectsPath => GetProjectListPath
  • DdevGlobalProjects => DdevProjectList
  • DdevGlobalProjectName => DdevProjectListFileName
    etc. If maintainers agree, I'll make those changes.

I'll admit I don't fully know what that list is used for - but running this locally everything seems to work as expected. I'll look at updating tests once maintainers have let me know if this is the right direction, and any general implementation changes they want.

Manual Testing Instructions

I'm honestly not sure what this list is for so I'm not sure what manual testing will actually show if it's working as expected.

Creating a new project did add it to the new file, .gotmp/bin/linux_amd64/ddev list worked as expected, and deleting the project removed it from the list.

Automated Testing Overview

To be done once I know if this is the right direction.

Related Issue Link(s)

#5639

Release/Deployment Notes

The source of truth for the project list remains in ~/.ddev/global_config.yaml.
This way, we ensure that this change is not breaking and people will be able to move from one version of DDEV to another without losing the project list.

Some time after a few new releases, when the vast majority of people have this new project file, project_list can be easily removed from ~/.ddev/global_config.yaml, leaving ~/.ddev/project_list.yaml as the only source.

Copy link

github-actions bot commented Dec 19, 2023

@stasadev
Copy link
Member

This is a tricky task because we also need to allow people to downgrade without losing the project list.

What if DDEV keeps the project list in two files for now and reads it from projects.yaml, and in some future version we remove project_info from global_config.yaml 🤔

As for the function and variable names, I agree that Global should be removed.

I'm honestly not sure what this list is for so I'm not sure what manual testing will actually show if it's working as expected.

It is used in many places, for example, it allows you to execute ddev start project-name from any folder - so it knows that you have a project-name (from the global configuration file).

@GuySartorelli
Copy link
Collaborator Author

What if DDEV keeps the project list in two files for now and reads it from projects.yaml, and in some future version we remove project_info from global_config.yaml 🤔

Sounds good to me, that should be pretty straight forward. The fact that most of the test runs are passing already is a good sign that we won't end up missing any reads.

I am a little worried about the two (non-doc) failures though - I'm not sure what is causing those failures, and I'm hesitant to make the swap to keeping the list in both places until we know everything will be using the new file for reading without causing problems. Could you please help me understand what's causing those failures?

@stasadev
Copy link
Member

I am a little worried about the two (non-doc) failures though

Colima fails all the time, restarted the test.

The panic in ddev-macos-amd64-mutagen looks scary, we had some issues with Docker Desktop in this runner, it may not be related to your changes, restarted the test.

@GuySartorelli
Copy link
Collaborator Author

GuySartorelli commented Dec 22, 2023

Awesome, I've updated the variables and file name and changed the logic to write to both global config and the new file.

Here's a scenario we don't currently handle:

  1. When upgrading, if there are any projects in project_info in global_config.yaml they will not be added to the new file, because we're not reading the project info from that file anymore.
  2. As soon as the project list is written to the new file for the first time, the data from project_info will be overwritten.

That's probably not ideal behaviour - my current thinking is in (or after) ReadGlobalProjects() we should check DdevGlobalConfig.ProjectList - if that list does not match what we just read from the new projects list file, we can assume that we've just upgraded from an earlier version and that the global config content is correct.

Does that sound about right?

Also, do you want any new tests for this? If so, it'll be my first time properly writing new tests (until now I've just copied existing ones that were very similar to what I needed) so I'll need some guidance for where to put them and what sort of thing you want to be tested.

@GuySartorelli GuySartorelli force-pushed the 20231219_GuySartorelli_projectslist branch from cc93fa8 to aaf0c89 Compare December 22, 2023 06:45
@stasadev
Copy link
Member

That's probably not ideal behaviour - my current thinking is in (or after) ReadGlobalProjects() we should check DdevGlobalConfig.ProjectList - if that list does not match what we just read from the new projects list file, we can assume that we've just upgraded from an earlier version and that the global config content is correct.

Sound right for me.

Also, do you want any new tests for this? If so, it'll be my first time properly writing new tests (until now I've just copied existing ones that were very similar to what I needed) so I'll need some guidance for where to put them and what sort of thing you want to be tested.

I've only written a few tests myself 🙂, looking for advice from @rfay here.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I see in tests seems like it will be fine, but I won't be able to look at this carefully until probably February.

@GuySartorelli GuySartorelli force-pushed the 20231219_GuySartorelli_projectslist branch from aaf0c89 to df378f6 Compare December 22, 2023 23:26
@GuySartorelli
Copy link
Collaborator Author

That's probably not ideal behaviour - my current thinking is in (or after) ReadGlobalProjects() we should check DdevGlobalConfig.ProjectList - if that list does not match what we just read from the new projects list file, we can assume that we've just upgraded from an earlier version and that the global config content is correct.

Sound right for me.

Sweet, I've done that. There might be a better way to compare maps - if so let me know and I'll make the change.

What I see in tests seems like it will be fine, but I won't be able to look at this carefully until probably February.

Sweet, no worries

@rfay
Copy link
Member

rfay commented Dec 23, 2023

I wasn't implying that it needs to wait for my attention. Stas is fully authorized to move forward on anything when he's comfortable with it.

pkg/globalconfig/global_config.go Outdated Show resolved Hide resolved
pkg/globalconfig/global_config.go Outdated Show resolved Hide resolved
pkg/globalconfig/global_config.go Outdated Show resolved Hide resolved
@stasadev stasadev force-pushed the 20231219_GuySartorelli_projectslist branch from 91184bd to ce7fdc3 Compare December 27, 2023 19:09
@stasadev
Copy link
Member

Rebased because the artifact bot comment links were invalid (see #5662).

@stasadev stasadev force-pushed the 20231219_GuySartorelli_projectslist branch from ce7fdc3 to f14e2dc Compare December 27, 2023 19:13
@GuySartorelli GuySartorelli force-pushed the 20231219_GuySartorelli_projectslist branch from f14e2dc to 0ed8ac3 Compare December 28, 2023 00:36
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, and confirmed that ddev config writes data to both files.

One more important issue, when you switch the binary and run ddev list and ddev start everything looks good.

But when you don't have ~/.ddev/project_list.yaml yet, replace the binary, and run, for example, ddev version, project_info is lost in ~/.ddev/global_config.yaml.

@GuySartorelli GuySartorelli force-pushed the 20231219_GuySartorelli_projectslist branch from 0ed8ac3 to ad040b9 Compare January 8, 2024 03:43
@GuySartorelli
Copy link
Collaborator Author

Good catch - I didn't notice that 'cause the ddev list command still shows those existing projects even though they're not in the file anymore (not sure where it's getting the info from...)

Should be fixed now.

@GuySartorelli GuySartorelli force-pushed the 20231219_GuySartorelli_projectslist branch from ad040b9 to 6d6cc74 Compare January 8, 2024 03:59
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great!

I've updated the Release/Deployment Notes in the first post.

@rfay
Copy link
Member

rfay commented Jan 9, 2024

DDEV inspects existing container labels for ddev list in addition to using the global config for this. So that's where things are coming from.

@stasadev
Copy link
Member

Thank you for all the hard work on this, merging.

@stasadev stasadev merged commit 94d7750 into ddev:master Jan 16, 2024
24 checks passed
@rpkoller
Copy link
Collaborator

awesome thank you for all the effort! already tested and i can confirm that right after the update ~/.ddev/project_list.yaml was created. with the identical number of projects listed in the global_config.yaml. at first i wanted to raise a question because of the redundancy between the two yaml files, but note to myself i should read the Release/Deployment Notes first properly ;) therefore instead of a question this confirmation that everything works and a thank you ;)

@GuySartorelli GuySartorelli deleted the 20231219_GuySartorelli_projectslist branch January 16, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants