Skip to content
This repository was archived by the owner on Apr 14, 2021. It is now read-only.

[2.0] Add a feature flag for bundle install not —cacheing by default#5724

Closed
segiddins wants to merge 2 commits intomasterfrom
seg-install-does-not-cache-by-default
Closed

[2.0] Add a feature flag for bundle install not —cacheing by default#5724
segiddins wants to merge 2 commits intomasterfrom
seg-install-does-not-cache-by-default

Conversation

@segiddins
Copy link
Copy Markdown
Contributor

What was the end-user problem that led to this PR?

The problem was that bundle install would default to always updating the cache in vendor/bundle (if it existed).

Was was your diagnosis of the problem?

My diagnosis was that the default for the --cache flag should be false instead of true.

What is your fix for the problem, implemented in this PR?

My fix is to invert the default on 2.0, using a feature flag. Taken from #3555.

Why did you choose this fix out of the possible options?

I chose this fix because it allowed a feature flag for a breaking change in 2.0 and was minimally invasive.

@segiddins segiddins changed the title Add a feature flag for bundle install not —cacheing by default [2.0] Add a feature flag for bundle install not —cacheing by default Jun 15, 2017
@bundlerbot
Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #5722) made this pull request unmergeable. Please resolve the merge conflicts.

@indirect
Copy link
Copy Markdown

r+

@segiddins segiddins force-pushed the seg-install-does-not-cache-by-default branch from 35fe424 to 8a5172d Compare June 19, 2017 01:53
@bundlerbot
Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #5727) made this pull request unmergeable. Please resolve the merge conflicts.

@segiddins segiddins force-pushed the seg-install-does-not-cache-by-default branch from 8a5172d to 20d945f Compare June 19, 2017 15:30
bundlerbot added a commit that referenced this pull request Jun 20, 2017
…lby-swandale

[CLI] Dont print defaults in the command printed with --verbose

### What was the end-user problem that led to this PR?

The problem was that running `bundle lock --verbose`  would print

    Running `bundle lock    --add-platform  --remove-platform  --verbose` with bundler 1.15.1

even though the platform flags were never specified. This was surfaced by #5724, which will be fixed by this PR.

### Was was your diagnosis of the problem?

My diagnosis was that default values of options were being printed.

### What is your fix for the problem, implemented in this PR?

My fix is to filter out the default values before getting Thor

### Why did you choose this fix out of the possible options?

I chose this fix because there is no way, within the `CLI` instance, to tell which flags the user has explicitly given, so I felt that filtering out those options that had default values was the next-best thing.
bundlerbot added a commit that referenced this pull request Jun 20, 2017
…lby-swandale

[CLI] Dont print defaults in the command printed with --verbose

### What was the end-user problem that led to this PR?

The problem was that running `bundle lock --verbose`  would print

    Running `bundle lock    --add-platform  --remove-platform  --verbose` with bundler 1.15.1

even though the platform flags were never specified. This was surfaced by #5724, which will be fixed by this PR.

### Was was your diagnosis of the problem?

My diagnosis was that default values of options were being printed.

### What is your fix for the problem, implemented in this PR?

My fix is to filter out the default values before getting Thor

### Why did you choose this fix out of the possible options?

I chose this fix because there is no way, within the `CLI` instance, to tell which flags the user has explicitly given, so I felt that filtering out those options that had default values was the next-best thing.
@bundlerbot
Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #5788) made this pull request unmergeable. Please resolve the merge conflicts.

@segiddins segiddins force-pushed the seg-install-does-not-cache-by-default branch from 20d945f to 20aa759 Compare July 2, 2017 13:42
@segiddins
Copy link
Copy Markdown
Contributor Author

Rebased. Only 1.0 failure was due to trampolining, which is now gone.

@segiddins
Copy link
Copy Markdown
Contributor Author

This causes a few changes of behavior that we need to decide if we want to fix or change:

https://travis-ci.org/bundler/bundler/jobs/249329317#L407

@bundlerbot
Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #5790) made this pull request unmergeable. Please resolve the merge conflicts.

@segiddins segiddins force-pushed the seg-install-does-not-cache-by-default branch from 20aa759 to aee6bcd Compare July 5, 2017 13:20
@segiddins
Copy link
Copy Markdown
Contributor Author

Rebased.

@segiddins segiddins force-pushed the seg-install-does-not-cache-by-default branch from aee6bcd to 1bb460b Compare July 5, 2017 13:40
@segiddins segiddins force-pushed the seg-install-does-not-cache-by-default branch from 1bb460b to ae0363d Compare July 5, 2017 13:40
@bundlerbot
Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #5839) made this pull request unmergeable. Please resolve the merge conflicts.

segiddins pushed a commit that referenced this pull request Jul 17, 2017
…lby-swandale

[CLI] Dont print defaults in the command printed with --verbose

### What was the end-user problem that led to this PR?

The problem was that running `bundle lock --verbose`  would print

    Running `bundle lock    --add-platform  --remove-platform  --verbose` with bundler 1.15.1

even though the platform flags were never specified. This was surfaced by #5724, which will be fixed by this PR.

### Was was your diagnosis of the problem?

My diagnosis was that default values of options were being printed.

### What is your fix for the problem, implemented in this PR?

My fix is to filter out the default values before getting Thor

### Why did you choose this fix out of the possible options?

I chose this fix because there is no way, within the `CLI` instance, to tell which flags the user has explicitly given, so I felt that filtering out those options that had default values was the next-best thing.

(cherry picked from commit de7d488)

# Conflicts:
#	lib/bundler/cli.rb
@segiddins
Copy link
Copy Markdown
Contributor Author

@indirect should I close this since we got rid of the --no-cache option on 2.0 already?

@segiddins
Copy link
Copy Markdown
Contributor Author

@indirect ping?

@indirect
Copy link
Copy Markdown

Now that the --cache/--no-cache flag is gone, let's drop this. Thanks!

@segiddins segiddins deleted the seg-install-does-not-cache-by-default branch August 29, 2017 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants