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

Allow running Textile client in no-key state #141

Merged
merged 4 commits into from
Sep 1, 2020
Merged

Conversation

dmerrill6
Copy link
Contributor

@dmerrill6 dmerrill6 commented Sep 1, 2020

Change log

  • Textile client is now started on boot but does not require a key. Initialization is delayed until there's key

  • Message channel so that textile can initialize defaults when key is added, and to reset to default state when key is removed.

  • Fixed delete key response

[ch17853]

Copy link
Contributor

@maurycy maurycy left a comment

Choose a reason for hiding this comment

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

@dmerrill6

Some quick, probably unsubstantiated, comments.

app/app.go Show resolved Hide resolved
core/textile/client.go Show resolved Hide resolved
core/textile/client.go Show resolved Hide resolved
core/textile/client.go Show resolved Hide resolved
Copy link
Contributor

@jsonsivar jsonsivar left a comment

Choose a reason for hiding this comment

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

LGTM. Just left some minor comments/questions.

err2 := kc.st.Remove([]byte(PublicKeyStoreKey))
// Note: currently ignoring error on keychain removal because it's failing randomly.
// Use GenerateKeyPair with override option instead.
ring.Remove(PrivateKeyStoreKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add a bug card with the exact error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// Starts a Textile Client and also initializes default resources for it like a key pair and default bucket.
// Then leaves the process running to attempt to connect or to initialize if it's not already initialized
func (tc *textileClient) Start(ctx context.Context, cfg config.Config) error {
// Create key pair if not present
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this leftover comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5660fd4

}

wasConnectedToHub := tc.isConnectedToHub

tc.isConnectedToHub = true

// setup mailbox
mid, err := tc.uc.SetupMailbox(hubctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually only needs to happen once (per keypair) so maybe this could get moved into the wasConnectedToHub == false case as well to avoid extra network calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5660fd4

@@ -125,7 +129,28 @@ func (tc *textileClient) start(ctx context.Context, cfg config.Config) error {
tc.ConnectToHub(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since ConnectToHub is now called via healthcheck, maybe we can remove this call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 5660fd4

tc.initialize(ctx)
}

tc.ConnectToHub(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we wrap this in a tc.isConnectedToHub == false clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated ConnectToHub to better reflect what it's doing right now. So now it's called checkHubConnection and it will get the token from the store, refresh it if necessary, and connect to mailbox if necessary. If any of these steps fail, tc.isConnectedToHub will be set to false.

@dmerrill6 dmerrill6 merged commit bf2045d into master Sep 1, 2020
maurycy added a commit that referenced this pull request Sep 2, 2020
…ate-mirror-bucket-on

* master:
  Set ulimit on start (#142)
  getMetaThreadContext: pass hub
  Allow running Textile client in no-key state (#141)
  Sharing: update accept/reject handlers to single func.
  don't block <-component.WaitForReady() on err
  Sharing: remove todo for message as we actually need to send it now
  Sharing: add proper placeholder for sharing API call.
  Merge master: fix go.mod
  Remove member struct
  Remove member collection pieces.
  Fix broken tests: update mock txl client
  Sharing: add todo for integrating buckets acl api
  Remove leftover comment in sharing domain
  Added message type to message
  Initial commit for sharing updates, left placeholder for textile acl integration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants