Actually switch the default behavior of github sources.#6911
Actually switch the default behavior of github sources.#6911jfly wants to merge 1 commit intorubygems:2-0-stablefrom
Conversation
|
Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack. For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide |
lib/bundler/dsl.rb
Outdated
|
|
||
| def warn_deprecated_git_source(name, replacement, additional_message = nil) | ||
| # TODO: 2.0 remove deprecation | ||
| # TODO: 3.0 remove deprecation |
There was a problem hiding this comment.
I'm not sure if this was the right thing to do, or if I should have actually removed/changed some code here. I'm guessing from the Bundler::SharedHelpers.major_deprecation 3 that someone changed their mind and decided to keep this around until Bundler 3?
There was a problem hiding this comment.
I think we should instead keep the sources, and enable the deprecations instead (Bundler::SharedHelpers.major_deprecation 2). I can do that in #6933, since that's what that PR is about.
|
I'm not sure if I should update CHANGELOG.md as well. Happy to do so if I should! |
jfly
force-pushed
the
issue-6910-change-github-source
branch
5 times, most recently
from
827e8a5 to
2b4263e
Compare
deivid-rodriguez
left a comment
deivid-rodriguez
left a comment
There was a problem hiding this comment.
I think it's better to limit this PR to the Bundler.feature_flag.github_https? change and I'll take care of the deprecations in #6933 if that seems ok with you!
lib/bundler/dsl.rb
Outdated
|
|
||
| def warn_deprecated_git_source(name, replacement, additional_message = nil) | ||
| # TODO: 2.0 remove deprecation | ||
| # TODO: 3.0 remove deprecation |
There was a problem hiding this comment.
I think we should instead keep the sources, and enable the deprecations instead (Bundler::SharedHelpers.major_deprecation 2). I can do that in #6933, since that's what that PR is about.
| repo_name = "#{repo_name}/#{repo_name}" unless repo_name.include?("/") | ||
| # TODO: 2.0 upgrade this setting to the default | ||
| if Bundler.settings["github.https"] | ||
| if Bundler.feature_flag.github_https? |
|
In general, I'm not sure of how we should do this transition, and other similar ones. In particular, this one is even more complicated because we are removing two different things: the source itself, and using https by default for it. If we are removing the source altogether, then the setting about using https by default seems redundant? In my opinion, the sources are fine and handy, as long as they use a secure default protocol? Thoughts? |
jfly
force-pushed
the
issue-6910-change-github-source
branch
from
2b4263e to
fd69b02
Compare
|
Thanks for the feedback, @deivid-rodriguez! Sorry for the delayed response, I was travelling. I have simplified this PR to focus just on the HTTPS default switch. I'll let you take care of the rest in #6933. |
jfly
force-pushed
the
issue-6910-change-github-source
branch
3 times, most recently
from
227f37b to
1807942
Compare
lib/bundler/dsl.rb
Outdated
| if Bundler.feature_flag.github_https? | ||
| "https://github.com/#{repo_name}.git" | ||
| else | ||
| Bundler::SharedHelpers.major_deprecation 3, "The `github.https` setting will be removed" |
There was a problem hiding this comment.
I think deprecating this setting now needs to be moved to https://github.com/bundler/bundler/blob/66e4215dbf87fedf2fdff3532a0742340c4e02f2/lib/bundler/feature_flag.rb, since here we can't be sure whether we are using the default value for the setting, or whether the user is actually setting it. The deprecation warning needs to be displayed only if the user is actually configuring the setting.
There was a problem hiding this comment.
Well, I moved this because I happen to know that the default for this setting is now true, so if we end up in this else block, then it means somebody changed it away from the default. This would not detect the case where someone has set it to "true", but that also doesn't seem terribly important to warn people about.
Alternatively, I could refactor feature_flag.rb a bit to add a warning if value is not nit here: https://github.com/bundler/bundler/blob/66e4215dbf87fedf2fdff3532a0742340c4e02f2/lib/bundler/feature_flag.rb#L22, but that would be a bit of shuffling, and I'm not sure it's worth the effort.
Whatever you think is best!
There was a problem hiding this comment.
Maybe the easiest (and still correct) thing to do for now is to change the deprecation text to: "Setting the `github.https` to false is deprecated and won't be supported in the future", or something like that?
There was a problem hiding this comment.
I like that! I've made this change.
jfly
force-pushed
the
issue-6910-change-github-source
branch
from
1807942 to
ffe4646
Compare
jfly
force-pushed
the
issue-6910-change-github-source
branch
from
ffe4646 to
7c6e79a
Compare
|
I have now have a plan on how to tackle these deprecations. For starters let's do this and I'll follow up from here! |
|
@bundlerbot r+ |
6911: Actually switch the default behavior of github sources. r=deivid-rodriguez a=jfly This fixes #6910. I also took this opportunity to remove some other code that the comments imply should have been removed in 2.0, but it looks like never actually got removed. Co-authored-by: Jeremy Fleischman <jeremyfleischman@gmail.com>
Timed out |
|
@bundlerbot retry |
6911: Actually switch the default behavior of github sources. r=deivid-rodriguez a=jfly This fixes #6910. I also took this opportunity to remove some other code that the comments imply should have been removed in 2.0, but it looks like never actually got removed. Co-authored-by: Jeremy Fleischman <jeremyfleischman@gmail.com>
|
Sounds good, thanks! |
Timed out |
|
We don't seem to have a setup to merge PRs to 2-0-stable. So for the moment I'm going to close this and I'll backport upon the next release. This is fixed (and your attribution kept) in #6975 anyways. |
|
Many thanks, @deivid-rodriguez! |
7142: Fully switch to https sources r=indirect a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that in #7000 I implemented a plan to migrate to https source by default and deprecate github/gist/bitbucket builtin sources, but I changed my mind and I think it's overkill. The reasons I changed my mind are: * We [actually announced the switch](https://bundler.io/blog/2019/01/03/announcing-bundler-2.html) and the fact that it didn't happen was not intented. So this can be considered as fixing a bug. * We have warned non https git sources for ages https://github.com/bundler/bundler/blob/a6533c0fe6541cc929f895ee0b7a9b673d34cb4d/lib/bundler/source_list.rb#L144-L153 I think people should be ready by now. Also, we introduced a new setting `github.https` that seems to do essentially the same thing as `git.allow_insecure`. ### What is your fix for the problem, implemented in this PR? My fix is to take the initial approach of #6911 and actually do this in the next release. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
3 participants
This fixes #6910. I also took this opportunity to remove some other code
that the comments imply should have been removed in 2.0, but it looks
like never actually got removed.