Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Allow bypassing branch check for local overrides #1810

Closed
wants to merge 1 commit into from

8 participants

@svenfuchs

Local overrides serve the purpose of making it easier to work with
multiple related repositories locally.

The current behaviour requires a branch name to be specified in the
Gemfile when working with local overrides. This makes sense as a quick
check as long as nothing else makes sure bundler is using the expected
refs in repositories we depend on.

I am working on an external tool that watches and displays the current
state of multiple related repositories. This tool uses bundler to check
and display the state a repository is in, but it is also supposed to
make it easier to quickly switch between, e.g., feature-branches in
various repositories. In this context having to manually edit the
Gemfile and add/change branch references rather defeats the purpose. The
tool itself is supposed to watch and make sure we use the correct
references from outside of bundler. Having bundler itself do a similar
check (that also requires manual editing of Ruby code) blocks the
intended workflow.

When discussing this with @josevalim he suggested to allow adding a flag
to the bundler settings in order to turn this check off.

The attached patch does just that.

@svenfuchs svenfuchs Allow bypassing branch check for local overrides
Local overrides serve the purpose of making it easier to work with
multiple related repositories locally.

The current behaviour requires a branch name to be specified in the
Gemfile when working with local overrides. This makes sense as a quick
check as long as nothing else makes sure bundler is using the expected
refs in repositories we depend on.

I am working on an external tool that watches and displays the current
state of multiple related repositories. This tool uses bundler to check
and display the state a repository is in, but it is also supposed to
make it easier to quickly switch between, e.g., feature-branches in
various repositories. In this context having to manually edit the
Gemfile and add/change branch references rather defeats the purpose. The
tool itself is supposed to watch and make sure we use the correct
references from outside of bundler. Having bundler itself do a similar
check (that also requires manual editing of Ruby code) blocks the
intended workflow.

When discussing this with @josevalim he suggested to allow adding a flag
to the bundler settings in order to turn this check off.

The attached patch does just that.
1b743e5
@josevalim
Collaborator

I am ok with this, we should just discuss if we could have a better name for the option. /cc @indirect @wycats

@svenfuchs

Cool! What's a better name?

:)

@indirect
Owner

Just to double check, you want this as a global setting, and not as a per-local-override setting, right? Maybe something like bundle config local_branches? I wish there was a way to do something like bundle config local.require_branch, but that conflicts with the local gem name scheme. Anyone else have ideas? :P

@svenfuchs

Yeah, it would be most convenient for me to have a global setting. It could be local settings, too, but that would be way more cumbersome :)

I was wondering why not have some nested keys in general like config.global.foo vs config.local.bar?

@rkh
rkh commented

:+1:

Started to use @josevalim's local repo feature and this is exactly what I need.

@indirect
Owner

Somewhat like git, bundler stores global state in ~/.bundle/config and local state in ./.bundle/config. So there's not really any reason to have global/local as part of the config name, I don't think.

Let's go with bundle config localbranches. If someone comes up with something better, we can switch to that before the final release.

@dlee

Can we disable branch checking by default?

I think it's a very common workflow to develop dependency gems at a feature branch, and then merge into master once the entire environment works.

Another common Gemfile usage pattern is to lock a git repository at a certain :ref or :tag, but that might collide if we by default require :branch.

@dlee

Sorry for the confusion pull request #1837 might have caused. I referenced the current Issue as a side note in that pull request, not realizing that Github would annotate that reference here. Pull request #1837 is unhelpful to the current discussion, so please treat it as a separate thing.

@travisbot

This pull request fails (merged 1b743e5 into b3e9c1a).

@rkh

Any updates on this? The new local git repos feature is really hard to use otherwise at the moment.

@indirect
Owner

@dlee, the entire reason we are so careful with branch checking by default is to make it harder to push something that is locked to a commit that doesn't exist. It's completely reasonable to change to the feature branch in your Gemfile if you're developing against it -- I do that all the time. It's very common to lock to a :ref or :tag, yes. If you do that, you can't use the local feature, by definition. Your Gemfile explicitly prohibits it, so Bundler will honor that.

@rhk, what's really hard to use? I would much rather get things usable that sweep them under the rug by letting everyone turn off Bundler's safeguards -- the safeguards are the main reason that Bundler exists, after all.

@rkh

Yes, it is really hard to use. Should I sent patches to all the projects I'm working with to add a branch setting? Safe-guard is the commit SHA1. And it should check the branch, if there is a branch specified. But checking the branch if no branch is set doesn't make much sense to me, like, at all.

@rkh

For the record, I'm actively using this feature and run into issues about three to four times a day.

@indirect
Owner

Requiring branches seemed like the best option at the time, and I know that both myself and José, at least, have been using this feature without any problems. If your sole problem is projects that don't have branches specified in their Gemfile, we might be able to fix that simply by tweaking this feature to acts the same as if you had put :branch => "master".

Can you please write a more detailed explanation of how you are using this feature and what issues you are running into? I'd like to make things work for everyone.

@svenfuchs

I don't think acting on no branch specified as if that means "i want master" doesn't help at all. The default usecase for me is that I simply want it to use whatever is checked out in my dependency repository locally.

@indirect
Owner

@svenfuchs, do you have any suggestions for how to avoid pushing code that is locked to commits that don't exist? I really don't like the git submodule "oh, you didn't do it right, so you're screwed" approach.

@rkh

@svenfuchs built a tool for that.

@dlee

@indirect Thanks for the response. I think I see where our disconnect lies. You're thinking of overrides as "override the git repo with a local repo, but don't change anything else, including branch specification". I'm thinking of overrides as "override the gem codebase with another codebase".

I think my interpretation of "override" is the more natural interpretation. Specifically, my interpretation is that the entire gem 'gemname', ... is overridden, whereas your interpretation is that only the :git => ... option is overridden. You could rename the config to override_git_path.gemname to reduce confusion, but I argue we should leave it broad.

I contend that the override feature I'm thinking of is more useful for development. You say that it's unsafe and will cause dependency disparity issues. But I would argue the whole point of overrides is to bust out of the locked down version/revision of a gem in order to test code in development. Overrides is a development feature, not a deployment feature, and users of the feature should understand that.

Furthermore, I think enforcing the branch is arbitrary, since it doesn't really safeguard against anything. Your local code is still running against uncommitted local code. Why does it matter what branch you're developing in? In fact, git best-practices say you should make changes in a separate branch that later gets merged into your deployment branch.

Finally, many Bundler deployments actually use refs and tags instead of branch specification for explicit versioning of git dependencies. This extremely useful feature is unusable to all those users in its current implementation.

@svenfuchs Does this somewhat represent your opinion as well?

@indirect
Owner
@dlee

@indirect As I mentioned in my previous comment, the :branch specification does not safeguard against shooting yourself in the foot. As the current feature behavior stands, it is still easy to push a commit that can't run anywhere except on your development machine.

Also, nobody is claiming that "safeguards are annoying and dumb, we should turn them off by default". Safeguards are useful, especially for deployment, but we can't have our cake and eat it too. The override feature specifically is an override of the safeguard. We are intentionally allowing users to run uncommitted code. You can't say we'll provide a feature to run against uncommitted code and at the same time have that feature safeguard you from running against uncommitted code.

I'm afraid that the :branch specification will provide users a false sense of security into thinking they're protected from pushing code that doesn't work anywhere except on their development machine. Perhaps you're falling prey to that as well?

@svenfuchs

i can just say that, in daily practice, the current feature is so annoying to me that i use a patched version of bundler and ask to be able to turn this stuff off by default, everywhere, at any time. So, I can totally understand why people ask for this to be turned off by default, but I'd still be super happy with a global flag that I can set.

@rkh

I do not see the connection between forcing a branch setting and forgetting to push changes.

@rkh

See #1934 for a simpler solution.

@indirect
Owner
@rkh

OK, but that's a different issue than forgetting to push things. And it would be solved by setting the branch explicitly, no?

@phil-monroe

+1 for not requiring a branch. Maybe it could be require a branch or SHA or tag to enforce restrictions in released code, but use uncommitted changes locally.

In fact, it would be way more useful for us at Identified to be able to use path override with tags instead. Here is what our preferred use case would be:

We have a rails app that uses a gem hosted on github for shared models and functionality. When we make changes to the gem, they should should be autoloaded as if we had just specified the path. Whenever a change is pushed to our gem's github repo, our CI tests it and it gets tagged if tests pass. Then we can update the Gemfile in our rails app to the tag that we know passes tests. Then we can push the rails app anywhere and it will work with the gem from github.

As @indirect is trying to ensure safeguards, I feel that this would give the best of both worlds. It ensures that there should be a valid commit somewhere for the gem to use when pushed to a machine without the local override, which is great for testing, production and front end developers who really never need to change the backend gem (and would constantly needed reminded to update the gem via git manually). As @dlee mentioned, only the developers who touch the gem opt out from the safeguard, and really what is the worse thing that could happen? The developer fires the rails app up in development, it doesn't work, they do a git pull on the gem, and hey things start working.

I feel that requiring a branch/SHA/tag on an override is cool and ensures that something should exists when the code is shared, but really limits developers who want things to update quickly. Safeguards are great, but they shouldn't increase development time drastically. Besides, with and without local override, you can push references to nonexistent code while still be providing a branch/SHA/tag. You can't fix all stupidity.

@indirect
Owner

Done -- I wound up pulling both @phil-monroe and @rkh's removal of the branch check. Everybody, does the current master branch satisfy your desired use-cases? Thanks for the discussion.

@indirect indirect closed this
@phil-monroe

I feel like the changes give the developer the freedom to be choose how they want to use it. Thanks for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 1, 2012
  1. @svenfuchs

    Allow bypassing branch check for local overrides

    svenfuchs authored
    Local overrides serve the purpose of making it easier to work with
    multiple related repositories locally.
    
    The current behaviour requires a branch name to be specified in the
    Gemfile when working with local overrides. This makes sense as a quick
    check as long as nothing else makes sure bundler is using the expected
    refs in repositories we depend on.
    
    I am working on an external tool that watches and displays the current
    state of multiple related repositories. This tool uses bundler to check
    and display the state a repository is in, but it is also supposed to
    make it easier to quickly switch between, e.g., feature-branches in
    various repositories. In this context having to manually edit the
    Gemfile and add/change branch references rather defeats the purpose. The
    tool itself is supposed to watch and make sure we use the correct
    references from outside of bundler. Having bundler itself do a similar
    check (that also requires manual editing of Ruby code) blocks the
    intended workflow.
    
    When discussing this with @josevalim he suggested to allow adding a flag
    to the bundler settings in order to turn this check off.
    
    The attached patch does just that.
This page is out of date. Refresh to see the latest.
Showing with 18 additions and 2 deletions.
  1. +1 −1  lib/bundler/source.rb
  2. +17 −1 spec/install/git_spec.rb
View
2  lib/bundler/source.rb
@@ -717,7 +717,7 @@ def local_override!(path)
path = Pathname.new(path)
path = path.expand_path(Bundler.root) unless path.relative?
- unless options["branch"]
+ unless options["branch"] || Bundler.settings['local_override_require_branch'] == 'false'
raise GitError, "Cannot use local override for #{name} at #{path} because " \
":branch is not specified in Gemfile. Specify a branch or check " \
"`bundle config --delete` to remove the local override"
View
18 spec/install/git_spec.rb
@@ -254,7 +254,7 @@
out.should =~ /Cannot use local override for rack-0.8 because #{Regexp.escape(lib_path('local-rack').to_s)} does not exist/
end
- it "explodes if branch is not given" do
+ it "explodes if branch is not given without being allowed to" do
build_git "rack", "0.8"
FileUtils.cp_r("#{lib_path('rack-0.8')}/.", lib_path('local-rack'))
@@ -264,10 +264,26 @@
G
bundle %|config local.rack #{lib_path('local-rack')}|
+ bundle %|config --delete local_override_require_branch|
bundle :install
out.should =~ /because :branch is not specified in Gemfile/
end
+ it "does not explode if branch is not given but settings allow this" do
+ build_git "rack", "0.8"
+ FileUtils.cp_r("#{lib_path('rack-0.8')}/.", lib_path('local-rack'))
+
+ install_gemfile <<-G
+ source "file://#{gem_repo1}"
+ gem "rack", :git => "#{lib_path('rack-0.8')}"
+ G
+
+ bundle %|config local.rack #{lib_path('local-rack')}|
+ bundle %|config local_override_require_branch false|
+ bundle :install
+ out.should_not =~ /because :branch is not specified in Gemfile/
+ end
+
it "explodes on different branches" do
build_git "rack", "0.8"
Something went wrong with that request. Please try again.