(CODEMGMT-1415) SPIKE assume modules unchanged locally #1145
Conversation
| @dirname = dirname | ||
| @owner, @name = parse_title(@title) | ||
| @path = Pathname.new(File.join(@dirname, @name)) | ||
| @version = find_version(args) |
There was a problem hiding this comment.
Do we need all these fields? Is there some hidden contract that requires them?
There was a problem hiding this comment.
We need the version and name in the current implementation (which requires the title). I thought we might need the path but I don't think so. This file was just a copy and pared down version of the module base class.
|
This is still a spike, but I'm opening it up as "ready for review" so that those subscribing can learn about it. Especially, regarding the efficacy of this change and the UX around |
| @modules << mod | ||
| # We want to manage the content (and not purge it) even if we do not want | ||
| # to sync modules that we assume unchanged. | ||
| no_change ? @modules : @modules << mod |
There was a problem hiding this comment.
I worry about focusing effort on the Puppetfile, when I see the future direction being more "modules attached to an environment", with the Puppetfile being kind of a legacy way to attach them.
Is it possible to take something like this approach instead?
- Add something like
#insync?and#insync=methods to module objects. Also#insyncto get the detailed value. - In the Puppetfile, set
mod.insync = true/false/:unknownas appropriate. When:unknown,#insync?returns false. - Instead of adding logic to mask the existence of the module here in the Puppetfile code, figure out where the module is actually synced/verified, and put an
... unless mod.insync?there. I think that might be in the environment deploy action (to pass e.g. an@verifyflag) and/or in the module sync method.
That approach would also preserve the ability to "see" all modules attached to an environment, regardless of what you're planning on doing to/with them.
Again, mostly I'm just worried about embedding important logic in the Puppetfile specifically, given that the Puppetfile is only one way to define environment content. At some point a general encapsulation/cleanup of how environments and modules relate would be nice, but in the meantime I think not implementing new stuff as Puppetfile-specific would be a good idea.
|
One thing I've noticed with this is that the environment info that is written will no longer include the full module list like it currently does. (when investigating this I also opened this PR to clean up writing that info a little: #1148) |
|
This PR has been marked stale because it has had no activity for 60 days. If you are still interested in getting this merged, please comment and we'll try to move it forward. Otherwise, it will be closed in 7 days. |
|
Not stale 🙂 |
mwaggett
added
do not merge
feature
has jira ticket
|
I think this has been superseded by #1200, going to close it. Please reopen if you still need it for something. |
Please add all notable changes to the "Unreleased" section of the CHANGELOG in the format: