search

claudiajs / claudia

Deploy Node.js projects to AWS Lambda and API Gateway easily
https://claudiajs.com
MIT License
3.8k stars 276 forks source link

fix missing tokenCodeFn #181

Closed gregory closed 5 years ago

gregory commented 5 years ago

I was missing the tokenCodeFn and allow to pass mfa serial through environment variable.

gojko commented 5 years ago

Hi, instead of readline, let's make this another command line argument?

gregory commented 5 years ago

What about I update to have both, so that it ll fallback to readline if not present in arguments?

gojko commented 5 years ago

that sounds ok to me.

gregory commented 5 years ago

addressed your suggestion @gojko ! Let me know if you need anything else. Cheers.

gojko commented 5 years ago

hi - thanks for this, looks good. I will test and merge over the weekend.

gojko commented 5 years ago

Hi - I merged your code and tried to add some specs around it so we can maintain it easier. can you please try if this branch still works for you: https://github.com/claudiajs/claudia/tree/gregory-fix_bug_with_mfa

if it does what you need, we can merge everything into master

gregory commented 5 years ago

sorry for the delay @gojko - that was a good idea. Code is cleaner & still works for me. Thanks, let me know if i can help with anything else.

gojko commented 5 years ago

@gregory thanks, will release this later today then

gojko commented 5 years ago

I can't publish this yet because the current aws-sdk release breaks claudia, I've reported a bug there and waiting on them to release it. as soon as that's done, I'll re-run the tests again and push a new release out

gregory commented 5 years ago

wow - crazy - can you link the bug you reported so that we can track status here? Thanks for the head's up!

gojko commented 5 years ago

@gregory the bug was https://github.com/aws/aws-sdk-js/issues/2588. I released claudia 5.4.2 just now, with your patch, so upgrading to the latest version should have this PR merged in