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

when running an external command, only construct a node if running in online mode #5194

Merged
merged 2 commits into from
Jul 5, 2018

Conversation

Stebalien
Copy link
Member

This breaks commands like ipfs update that expect IPFS to not be running.

fixes #5191

@Stebalien Stebalien requested a review from Kubuxu as a code owner July 5, 2018 21:27
@ghost ghost assigned Stebalien Jul 5, 2018
@ghost ghost added the status/in-progress In progress label Jul 5, 2018
This breaks commands like `ipfs update` that expect IPFS to *not* be running.

fixes #5191

License: MIT
Signed-off-by: Steven Allen <[email protected]>
License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien Stebalien changed the title don't automatically construct a new node when running external commands when running an external command, only construct a node if running in online mode Jul 5, 2018
@Stebalien Stebalien added need/review Needs a review and removed status/in-progress In progress labels Jul 5, 2018
if err == nil {
// Get the node iff already defined.
if req.InvocContext().Online {
nd, err := req.InvocContext().GetNode()
Copy link
Member

Choose a reason for hiding this comment

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

we can probably just skip this GetNode call entirely and just use the req.InvocContext().Online value directly below for setting the variable

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to ensure we actually created the node. Online means "node will be constructed in online mode" but I don't know if it actually constructs the node. Actually, I'm pretty sure it doesn't.

@whyrusleeping whyrusleeping merged commit 48577e0 into master Jul 5, 2018
@whyrusleeping whyrusleeping deleted the fix/5191 branch July 5, 2018 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

repo migration failing when attempting to update to ipfs v0.4.16-rc1
2 participants