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

Introduce state management package to keep state of messages feature and global state #4948

Merged
merged 1 commit into from
May 24, 2023

Conversation

gilbertsoft
Copy link
Member

@gilbertsoft gilbertsoft commented May 23, 2023

The Issue

State is currently mixed into configuration but should be separated into a dedicated state file instead. E.g. there is some state in the globalconfig ~/.ddev/global_config.yml namely the project infos or last started version.

How This PR Solves The Issue

This PR introduces a small state management package which makes state handling easy possible by defining e.g. a state struct. The package supports the management of multiple states completey independant, means no cross dependencies are added from a state to another, a single state works completely independant. This ensures a clean per package state management but in a central place for the whole application within one state file. A state is accessed by his unique key.

  • this package will be used by Feature showing messages to the users, fixes #3799 #4885 which e.g. needs to know about the last update or the last message which was shown.
  • also a PR will follow which decouples the state from the globalconfig e.g. project infos or last started version which should not be stored in the config but a separate state file.

PR should be merged as soon as the tests are completed.

Manual Testing Instructions

No manual testing is possible at the moment because the code is not used so far.

Automated Testing Overview

The packages are fully automated tested with multiple unit tests and for the YAML state storage also benchmark tests are included.

Related Issue Link(s)

Release/Deployment Notes

No side effects are possible at the moment because the code is not used so far.

@github-actions
Copy link

github-actions bot commented May 23, 2023

@rfay
Copy link
Member

rfay commented May 23, 2023

Please add to the OP a practical statement about what this will be used for and what it means. You'll need to explain "state management" and its impact on existing code.

Also, please say whether this could be introduced in the near term or whether it needs to wait until after our major release.

@gilbertsoft gilbertsoft force-pushed the task/introduce-state branch 2 times, most recently from ca50cb1 to 03d7957 Compare May 23, 2023 13:18
@gilbertsoft
Copy link
Member Author

Please add to the OP a practical statement about what this will be used for and what it means. You'll need to explain "state management" and its impact on existing code.

Also, please say whether this could be introduced in the near term or whether it needs to wait until after our major release.

Done. As written above code is currently not in use but a preparation for #4885.

@gilbertsoft gilbertsoft marked this pull request as ready for review May 23, 2023 13:29
@gilbertsoft gilbertsoft requested review from a team as code owners May 23, 2023 13:29
@rfay
Copy link
Member

rfay commented May 23, 2023

What I'm asking for is a practical description of what this does and why, and how it will change other code. Please choose an example command/state/config and explain how this will change that.

@gilbertsoft
Copy link
Member Author

What I'm asking for is a practical description of what this does and why, and how it will change other code. Please choose an example command/state/config and explain how this will change that.

sth like this? https://github.com/ddev/ddev/pull/4948/files#diff-102188c33aa91417b0ad17f55fa004a97b79d743024b7be5c1db269da3380076

@rfay
Copy link
Member

rfay commented May 23, 2023

I'm asking you to improve the PR OP above with a practical example, taken from DDEV, of how this changes things. Explain what this means to DDEV, what it changes, what motivates it.

@rfay
Copy link
Member

rfay commented May 23, 2023

If my words in this PR don't make any sense I'm happy to do a call with you and we can do question-and-answer to get this out.

@rfay rfay changed the title Introduce state management package Introduce state management package to keep state of messages feature and global state May 23, 2023
@gilbertsoft gilbertsoft force-pushed the task/introduce-state branch 2 times, most recently from d5bc45a to 8c40d2f Compare May 23, 2023 15:01
@rfay
Copy link
Member

rfay commented May 23, 2023

https://github.com/mitchellh/mapstructure looks like a wonderful thing. Could be used several other places.

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.

Thanks, I'll go with it! You can merge when you think it's ready. I usually try to make sure that the tests on master have succeeded before merging something new; the last two should be getting there pretty soon, but that latest commit there isn't very important (tests only) and the last two tests don't even use mutagen.

pkg/config/state/state_manager_test.go Show resolved Hide resolved
pkg/config/state/storage/yaml/yaml_storage.go Show resolved Hide resolved
@gilbertsoft
Copy link
Member Author

https://github.com/mitchellh/mapstructure looks like a wonderful thing. Could be used several other places.

Yes, makes some things easier and more flexible compared to the json and yaml implementation.

@gilbertsoft gilbertsoft merged commit dcce318 into ddev:master May 24, 2023
19 checks passed
@gilbertsoft gilbertsoft deleted the task/introduce-state branch May 24, 2023 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants