Conversation
amcgee
left a comment
There was a problem hiding this comment.
This is cool! I'm not sure if we want to go all the way to implementing a generic key-value store, but if we don't (yet) this is a great solution.
I don't think the multi-tiered Caches are required, which would make this even simpler!
e565467 to
91337d9
Compare
|
Rebased on Produces this structure: edit: updated with simpler paths. Depends on dhis2/cli-helpers-engine#16 |
I think I'll rename the edit: updated. |
|
Updated dhis2/cli-helpers-engine#16 with a The cached configuration reading/writing is handled after the runtime configuration has been resolved. Only a subset of the configuration object is cached for subsequent runs. |
|
@amcgee I think this is ready for review and testing without using |
046590e to
5e42b86
Compare
|
@varl looks like there's been a bunch of activity since you said this was ready for review, are you still iterating on it? |
|
@amcgee No, I just rebased on master to bring it up to date. Looks like there's another conflict so I'll do it once more. 😇 |
601fb46 to
83d6504
Compare
3f849e8 to
2d6209c
Compare
|
Rebased on |
|
@amcgee This is ready for review. |
amcgee
left a comment
There was a problem hiding this comment.
Looking good! Made a few comments
|
@amcgee changed the code with regards to your comments. |
| const cfg = await resolveConfiguration(argv) | ||
|
|
||
| const cacheLocation = await initDockerComposeCache({ | ||
| composeProjectName: name, |
There was a problem hiding this comment.
Should this also be makeComposeProject(name) or the parameter just called name? Otherwise maybe initDockerComposeCache::composeProjectName is a bit misleading...
There was a problem hiding this comment.
Yeah, I've considered that too.
The main reason I want name here is to make the cache a lot easier to browse: d2 debug cache list clusters/dev/docker-compose
If we use makeComposeProject: d2 debug cache list clusters/d2-cluster-dev/docker-compose
Since all the compose projects are already namespaced to clusters/{name} I don't think it makes sense to prefix it again with d2-cluster-{name}.
It's still the cache directory for the compose project, so the function name names sense (initDockerComposeCache), it's just the argument that is slightly misleading. I can rename it to composeCacheName if that makes it clearer.
There was a problem hiding this comment.
@amcgee Alternatively initDockerComposeCache::cacheDirName?
There was a problem hiding this comment.
We can rename this later but I think name might even be fine
amcgee
left a comment
There was a problem hiding this comment.
Testing and looks good but found one undefined variable!
|
No worries, the PR is not that big but the implications of it are huge so good to be thorough! |
# [1.3.0](v1.2.4...v1.3.0) (2019-07-01) ### Features * decouple configs from cluster ([#53](#53)) ([e5b40af](e5b40af))
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Creates a new Cache for
d2-clusterbased on the global cache.Would be nice to pair with https://github.com/dhis2/cli-helpers-engine/pull/15/files to remove the layered cache directory.