Add default prune options for init cmd #1460
Conversation
arbourd
changed the title
|
I've changed the destination branch to |
manifest.go
Outdated
| @@ -416,6 +416,23 @@ func fromRawPruneOptions(raw rawPruneOptions) (gps.PruneOptions, gps.PruneProjec | |||
| return rootOptions, pruneProjects | |||
| } | |||
|
|
|||
| func toRawPruneOptions(options gps.PruneOptions) rawPruneOptions { | |||
There was a problem hiding this comment.
Let's add a unit test. I realize we have integration tests that are tangentially testing this, but they don't exercise all of the flags.
There was a problem hiding this comment.
After this is updated to reflect the changes in #1219 , how can we clarify that this only works during the initial init? Unmarshalling project prune options is not possible without too much hacking...
There was a problem hiding this comment.
@ibrasho I am not sure I understand the question/problem?
There was a problem hiding this comment.
Apologies for not explaining thoroughly. 😅
Do you remember the issue with unmarshalling prune project options [1] [2] (since we have a mixed string/bools TOML table)?
In short, this function (while it's only used in init) can be used later and assumed to write the parsed manifest as-read. While in reality, it would at best ignore prune project options.
This might not be an issue now, but I want us to make sure that we don't forget this later and assume that this function can un-marshall the full prune options.
There was a problem hiding this comment.
Just an observation: this function is also called during ensure -add.
There was a problem hiding this comment.
if we have a way of encoding that failure mode as a panic, that would be great. I'm not sure offhand if it's possible, but something like "check if the map of project-specific options is non-empty,and if so, panic"
There was a problem hiding this comment.
yeah...the call during the new mode is troubling. really, that should only be operating on discrete, additional items, not a whole Manifest.
There was a problem hiding this comment.
@ibrasho Let me repeat back what I am hearing to make sure I've got it.
The concern is that this function only writes global prune options. If later someone tried to read in the Gopkg.toml, modify the in-memory manifest, and then write back the entire manifest to disk that the project level prune options would be lost?
I read through the code for dep ensure --add and have seen the manifest output from this PR. It doesn't appear to be behaving incorrectly. So I'd like to set that aside for now.
If you agree that I've finally understood your concern(?), then my initial preference would have been to fully implement the conversion from a manifest to toml. Unfortunately, it appears from your comment that the conversion of project-level prune options to a raw representation is not easy and would require hacks. I'll take your word on that! 😀
Since the full implementation is not actually needed, Sam's suggestion is to raise a panic when project-level prune options are present, forcing the developer to deal with this in the future if they try to use the unimplemented functionality.
Let me know if I've summarized correctly. If I have, then yes, I would also prefer a panic (and perhaps a copy/paste of my summary or a link back to it), over just a comment.
There was a problem hiding this comment.
A panic is a better and clearer approach.
|
As soon as we have a test, I'll merge into our release branch and then integrate it with #1219 when that's ready so that we are executing prune at the end of init. |
|
I rebased this PR on new branch and I actually have build errors based on what @ibrasho was mentioning. Will need to make another quick change. |
|
Updated with a test with a couple of cases, and fixed build errors. |
manifest.go
Outdated
| @@ -479,7 +495,7 @@ func (m *Manifest) toRaw() rawManifest { | |||
| } | |||
| sort.Sort(sortedRawProjects(raw.Overrides)) | |||
|
|
|||
| // TODO(ibrasho): write out prune options. | |||
| raw.PruneOptions = toRawPruneOptions(m.PruneOptions.PruneOptions) | |||
There was a problem hiding this comment.
I would recommend passing m.PruneOptions instead. So we can check if the projects are empty or not in toRawPruneOptions.
manifest.go
Outdated
| @@ -417,6 +417,23 @@ func fromRawPruneOptions(raw rawPruneOptions) gps.RootPruneOptions { | |||
| return opts | |||
| } | |||
|
|
|||
| func toRawPruneOptions(options gps.PruneOptions) rawPruneOptions { | |||
There was a problem hiding this comment.
Can you change it to accept a gps.RootPruneOptions, so we can check if options.ProjectOptions is not empty?
| @@ -15,7 +15,7 @@ import ( | |||
|
|
|||
| "github.com/golang/dep/gps" | |||
| "github.com/golang/dep/gps/pkgtree" | |||
| "github.com/pelletier/go-toml" | |||
| toml "github.com/pelletier/go-toml" | |||
What does this do / why do we need it?
initWhat should your reviewer look out for in this PR?
init.gotxn_writer.goDo you need help or clarification on anything?
Do we need tests for
toRawPruneOptions?Which issue(s) does this PR fix?
fixes #1404