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

Expose the ability to inject a custom DynamoDB client #140

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@daemonsy
Copy link

daemonsy commented Jun 27, 2017

  • Imagined use case: TDD using dynamoDB local
  • Set your own client using <Alexa Handler>.dynamoDBClient
  • If not set, it uses the default client to keep things convention over configuration
Expose the ability to inject a custom DynamoDB client
- Imagined use case: TDD using dynamoDB local
- Set your own client using `<Alexa Handler>.dynamoDBClient`
- If not set, it uses the default client to keep things convention over configuration
@abhilash1in

This comment has been minimized.

Copy link

abhilash1in commented Jul 28, 2017

Does the official SDK support alexa.dynamoDBClient?

@mblaya

This comment has been minimized.

Copy link

mblaya commented Nov 2, 2017

It would be great to have the dynamo handler available.
I am trying to use a separate table for some application data but I cannot (or I don't know how to) manage it. After a couple of days of tries and searches, I got here.
Any luck anybody finding a workaround ?

@neilsmind

This comment has been minimized.

Copy link

neilsmind commented Nov 17, 2017

@daemonsy Did this go anywhere somewhere else? It looks like the PR now has conflicts. Would love to be able to test locally using mocha but issue #139 was closed two weeks ago by @Amazon-tianrenz

@daemonsy

This comment has been minimized.

Copy link

daemonsy commented Nov 17, 2017

@neilsmind when I made the PR originally, it worked fully for testing with any framework. Just allowed you to stub away the dynamo client. But so much time passed now, I'm not sure.

Happy to resolve the conflicts and make it work again, but would like to know Amazon's stance on the issue / PR. Haven't heard anything from the maintainers yet.

@neilsmind

This comment has been minimized.

Copy link

neilsmind commented Nov 17, 2017

@daemonsy Thanks for the quick reply! I just heard from @Amazon-tianrenz on #134 with the following (when asked if they wanted help getting this PR's conflicts resolved):

I've created action item on internal backlog to track #139 and labeled it as "feature request". Please move to #139 for further discussion on local DynamoDB testing.
We are constantly reviewing PRs so it would be great if you can update the PR #140 with the latest SDK.

So it would seem that they're aware of the issue (#139) and if we can get the PR cleaned up, we may be able to get it reviewed / merged(?). I have a thought / question on the PR but for context, I'll add as a PR review.

@ultradian

This comment has been minimized.

Copy link

ultradian commented Dec 18, 2017

Hi. I'm still new to the process here, so any comments about my comments or documentation is appreciated. I continued the work of @daemonsy and just submitted a PR #242 which resolved the conflicts and also edited alexa.js to allow definition of a local dynamoDB client like:

alexa.dynamoDBClient = new aws.DynamoDB({
    accessKeyId: 'AKID',
    secretAccessKey: 'SECRET',
    region: "us-east-1",
    endpoint: "http://localhost:8000",
    apiVersion: '2012-08-10'
});

I have mainly been using the testflow.js program from the alexa-cookbook, so not really doing any true TDD and am unable to say if this works with any other frameworks. I hope this helps move the process forward and again appreciate any information about how I can better shape my contributions to be the most effective.

@daemonsy

This comment has been minimized.

Copy link

daemonsy commented Dec 26, 2017

Hey @ultradian sorry for the radio silence, didn't see the notifications. I updated the PR and resolved the merge conflicts, but my WIP tests in an alexa skill isn't passing. Will investigate.

@ultradian

This comment has been minimized.

Copy link

ultradian commented Dec 30, 2017

Hi @daemonsy. No worries re: speed of response. You are a lot faster than many! I just noticed that #139 is still open, so am trying to move things there. Let me know if you think my minor changes in PR #242 should merge with yours or vice versa to clean some stuff out.

@tianrenz

This comment has been minimized.

Copy link
Contributor

tianrenz commented Jan 2, 2018

Hi all,
Thanks for everyone's discussion on this thread. I'll throw in some of my suggestions here too.

Currently behavior is that AttributesHelper spawns a new aws.DynamoDB.DocumentClient in every set and get call. So try to injecting a new DynamoDB Client seems a bit redundant. If the goal here is to enable choice for configuring the DBClient for Local testing or switching regional endpoint, a simpler fix (from my point of view) would be to inject a DynamoDB configuration JSON and pass that into the AttributesHelper.

This way, developers using the SDK can avoid the boilerplate code to introduce a new dependency (aws-sdk) and actually create the DB client.

We have a task currently in backlog to add this feature. It will be enabled in future updates. Stay tuned!

Regards

@ultradian

This comment has been minimized.

Copy link

ultradian commented Jan 11, 2018

@Amazon-tianrenz That sounds like a great idea. I'm looking forward to seeing it implemented. When that happens, how is the public usually notified? Is it just here on Github or it get pushed into the documentation at https://developer.amazon.com/docs/custom-skills ?

@tianrenz

This comment has been minimized.

Copy link
Contributor

tianrenz commented Jan 11, 2018

Hi all,

Thanks for everyone's attention on this issue. The latest release has enabled developer to inject their own dynamoDB client into SDK.

Regards

@tianrenz tianrenz closed this May 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment