search

bmizerany / vendor

Vendor copies go dependencies to ./vendor
52 stars 6 forks source link

allow mode of copies to be set #8

Open djspoons opened 8 years ago

djspoons commented 8 years ago

It's be great if I could do something like vendor -mode 644 main.go so that I (or my teammates) don't start accidentally editing the vendored code.

bmizerany commented 8 years ago

I'm compelled to ask why you want this instead of failing in CI? I did a quick chmod 644 ./vendor and then git status and see: vendor/golang.org/x/oauth2/transport.go: Permission denied for every file vendored. It seems like CI failing would be more ideal and it also means one less feature in vendor.

djspoons commented 8 years ago

Sorry, I meant 444 (r--r--r--) on the copied files, and not the directories. My goal is just create a (small) obstacle to editing files in vendor/ so that I don't accidentally do some work in one, then run vendor only to have it overwrite my changes. If they are read-only, I will notice before I start editing.

There are other ways to do this (rm -fr vendor/ && vendor . && chmod -R a-w vendor). I'm curious: what would you check in CI?

bmizerany commented 8 years ago

I'm not sure what to check in CI actually. It was just a thought.

Maybe we can make a read-only flag but even then one would need to be sure to use it when vendoring. Maybe if a ./vendor/readonly file that can checked into source exists, vendor will make all copied files readonly.

Thoughts? On Wed, Aug 10, 2016 at 9:06 PM spoons notifications@github.com wrote:

Sorry, I meant 444 (r--r--r--) on the copied files, and not the directories. My goal is just create a (small) obstacle to editing files in vendor/ so that I don't accidentally do some work in one, then run vendor only to have it overwrite my changes. If they are read-only, I will notice before I start editing.

There are other ways to do this (rm -fr vendor/ && vendor . && chmod -R a-w vendor). I'm curious: what would you check in CI?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bmizerany/vendor/issues/8#issuecomment-239067626, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAALvB14RhgeqgSwrl4yzJeSIt8GdHoks5qep_QgaJpZM4IaZbq .

djspoons commented 8 years ago

Yeah, I appreciate that a flag could be forgotten. A readonly file would be cool... though there's some sort of slippery slope here to a config file. (That is, we create a file called ./vendor/config.yaml that contains something like readonly: true.) Also seems to stray from the minimal aesthetic, but you're the boss.

What do you think about making readonly the default? Do you have a workflow where you manually edit the files in ./vendor/?

djspoons commented 8 years ago

Actually, maybe the simplest thing is just to copy the relevant bits from the ./vendor/ directory itself? That is, drw-r--r-- 4 spoons staff 136 Aug 10 20:53 vendor would mean that copied files would be 644 dr--r--r-- 4 spoons staff 136 Aug 10 20:53 vendor would mean that copied files would be 444. No config file, no flag.

Let me see if I can make that work.

bmizerany commented 8 years ago

That sounds like a plan. I got a little heartburn when I offered up the readonly file anyway.

djspoons commented 8 years ago

https://github.com/bmizerany/vendor/pull/11 is slightly larger than I'd like (due to handling some corner cases like 'what if there's nothing to vendor?'). Thoughts? I still like the theory of this.

kr commented 7 years ago

Sorry to jump in from the peanut gallery here, but if the problem is "we don't want to accidentally modify our vendored code" I suspect a more effective solution would be to have someone or something watch for diffs in /vendor and warn or reject commits that change those files. (Plus, there's nothing wrong with editing vendored code as long as you don't commit your changes. I do that all the time when debugging.)

djspoons commented 7 years ago

Peanuts welcome. Here's the way I think about it:

kr commented 7 years ago

I want a hint to devs on my team that "if you are editing in /vendor, you should know that you are editing in /vendor; these files are special"

Ah, I see. I guess I was suggesting that perhaps a better time to provide that hint is during code review. But I can understand wanting it to happen earlier.

(On the other hand, I think sometimes people treat vendored files a little too gingerly. The only real danger is in accidentally committing behavior changes that cause problems later. But git does a really effective job at both making it easy to spot this before it happens during code review, and also at tracking it down afterward with git blame. So I don't totally buy that anyone needs a different mindset when they're editing in /vendor. In my opinion it would be better not to interrupt someone's flow. When they're stepping through code and want to make a tweak here or there during debugging, that's when they shouldn't have to worry about inconsequential differences like who wrote the file they're in (a 3rd party vs their own team). They or their reviewer should check for and clean up such things later, when the patch is ready. But that is just my personal philosophy, haha.)

Assuming that chmod is a good approach, I have one other observation about implementation strategy. If I were @bmizerany I would be wary of adding a flag to this tool when there's already another tool that does exactly what you want: chmod -R -w vendor. I suppose it would make sense to have the vendor tool always mark files readonly (without a flag) if this were something that most people wanted, but that seems unlikely.

kr commented 7 years ago

The only real danger is in accidentally committing behavior changes that cause problems later.

Sorry, that wasn't right. Even in that case, debugging is not much different from debugging problems in your own code. But committing changes to vendored code makes it harder to update later.