Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gateway: fix --writable flag :| #3206

Merged
merged 1 commit into from
Sep 14, 2016
Merged

gateway: fix --writable flag :| #3206

merged 1 commit into from
Sep 14, 2016

Conversation

ghost
Copy link

@ghost ghost commented Sep 11, 2016

Broke it in #2874... my unproven assumption apparantly was that cfg.Gateway.Writable = true would somehow magically make it into other instances of the config retrieved via Repo.Config()...

We're not overloading the repo config with this anymore now. We also have a test for --writable now.

@ghost ghost added need/review Needs a review regression status/in-progress In progress labels Sep 11, 2016
@ghost
Copy link
Author

ghost commented Sep 11, 2016

Added a test for --writable=false too

@ghost ghost added the topic/gateway Topic gateway label Sep 11, 2016
@ghost
Copy link
Author

ghost commented Sep 11, 2016

We can defer this to 0.4.4 since the workaround is to simply set Writable=true in the config. I imagine --writable was introduced as a shorthand for ipfs config --bool Gateway.Writable true.

test_kill_ipfs_daemon

test_launch_ipfs_daemon --writable=false
test_expect_success "ipfs daemon --writable=false overrides Writable=true config" '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we set the config value to true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks! Moved the call to test_config_ipfs_gateway_writable up a bit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not seeing it... Does it happen in one of the test libs?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey I think you might have been viewing the outdated diff -- it happens right there: https://github.com/ipfs/go-ipfs/pull/3206/files#r78514885

(and I filed #3208 for the unrelated test failure)

@ghost ghost force-pushed the feat/writable-flag branch 2 times, most recently from 4f06f49 to 11121b4 Compare September 11, 2016 18:49
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost
Copy link
Author

ghost commented Sep 11, 2016

Okay we're finally green, I filed #3208 for an unrelated failure.

if writableOptionFound {
cfg.Gateway.Writable = writable
if !writableOptionFound {
writable = cfg.Gateway.Writable
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 4 lines are the essence of the bug.

@whyrusleeping whyrusleeping assigned ghost Sep 14, 2016
@whyrusleeping
Copy link
Member

LGTM!

@whyrusleeping whyrusleeping merged commit 0aa136a into master Sep 14, 2016
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Sep 14, 2016
@whyrusleeping whyrusleeping deleted the feat/writable-flag branch September 14, 2016 17:50
@ghost ghost mentioned this pull request Dec 23, 2016
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
gateway: fix --writable flag :|

This commit was moved from ipfs/kubo@0aa136a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review regression topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant