Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

cmd/dep: clarify ensure help text #404

Closed
wants to merge 1 commit into from
Closed

Conversation

peterbourgon
Copy link
Contributor

A very tiny (but important) update to the help text: ensure no longer modifies the manifest file.

ensure no longer modifies the manifest file.
@carolynvs
Copy link
Collaborator

It may only modify the manifest when -update is specified.

if !cmd.update {

Though to be honest I'm not quite sure if that will ever result in the contents of manifest being altered (other than reformatting)?

@peterbourgon
Copy link
Contributor Author

I was under the impression that the tooling treats the manifest as read-only, TOML formatting fixes notwithstanding.

@carolynvs
Copy link
Collaborator

carolynvs commented Apr 20, 2017

So I was the villain who changed this in #235 (review). But I'm not sure anymore why I left this in:

https://github.com/golang/dep/blob/master/cmd/dep/ensure.go#L163

if !cmd.update {
		manifest = p.Manifest
}

It does result in minor tweaks to the manifest, such as in TestIntegration/status/case1, rewriting the following:

[[dependencies]]
  name = "github.com/sdboyer/deptest"
  version = "^0.8.0"

to this

[[dependencies]]
  name = "github.com/sdboyer/deptest"
  version = ">=0.8.0, <1.0.0"

I think those lines are a bug from my PR, and should be removed. If @sdboyer confirms, I will make an issue and fix.

@peterbourgon
Copy link
Contributor Author

What a pickle this is.

@carolynvs
Copy link
Collaborator

Gosh, I apologize for all the confusion I've caused! 😞 I've spent more time today that I am willing to admit trying to remember when (if ever) dep should modify the manifest file now that it's a user managed file...

I'm pretty sure that I've left things in a bad spot and will clean up my mess once I understand the desired behavior.

@sdboyer
Copy link
Member

sdboyer commented Apr 21, 2017

@carolynvs oh no, it's no big deal - this should be an easy fix. Yes, @peterbourgon's right - the only command that can ever fully write out the manifest is dep init. -update is only about changing what's in the lock.

The new spec in #277 contemplates a dep ensure -add that could append to the manifest - but never fully write it back out.

@carolynvs
Copy link
Collaborator

carolynvs commented Apr 21, 2017

I opened #454 to fix this goof on my part, and then realized maybe wasn't a goof? I was just preserving the existing behavior, which isn't the same in the new command spec.

I can't remove modifying the manifest during ensure as long as we still support dep ensure github.com/pkg/foo@^1.0.1, dep ensure github.com/pkg/foo:git.internal.com/alt/foo, and dep ensure -override github.com/pkg/foo@^1.0.1. Let me know if I should close #454 or just updated it to make it clear that it's part of #277.

@sdboyer
Copy link
Member

sdboyer commented Apr 22, 2017

@carolynvs hm, yeah, on further thought, i realize, yes - these really are two separate changes. We'd conflated a bunch of things together with respect to the move to TOML, but really, the issue is that we didn't lay out appropriate follow-up issues after the merge.

Moving away from re-writing out the manifest is separate from moving to TOML from JSON. And, until we implement the changes to create a new dep ensure in #277...well, it might be better to NOT make this change, as it would make the current-implemented approach to dep ensure make less sense.

So, I guess we put this on hold, and try to get to a new version of dep ensure ASAP.

@sdboyer
Copy link
Member

sdboyer commented Apr 28, 2017

Incorporated by #480

@sdboyer sdboyer closed this Apr 28, 2017
@sdboyer sdboyer deleted the clarify-ensure-help branch September 19, 2017 00:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants