search

Nordstrom / artillery-plugin-aws-sigv4

Plugin for artillery.io that signs HTTP requests using the AWS Signature V4 specification.
Apache License 2.0
16 stars 21 forks source link

Rewrite in ES6 #9

Closed dougmoscrop closed 7 years ago

dougmoscrop commented 7 years ago

Sooo the jscs rules didn't pass on the existing code, I'm really confused about how that happened, but anyway, I "fixed" most of them (except the one rule I disabled, which I could not make happy).

I fixed a bug in the original impl (it used .uri instead of .url) and ensured AWS config was loaded.

erikerikson commented 7 years ago

Sorry for the delay and thank you for this. Taking a look.

erikerikson commented 7 years ago

And explanation of that rule is here: http://jscs.info/rule/requireMultipleVarDecl.html

Basically, it declares the declaration of variables like so:

var a,
    b,
    c;

And at the top of the scope.

We've moved on, generally speaking from these rules, using eslint and a flavor thereof... so apologies for the broken state of master and legacy. Not sure how that happened but thanks for working past it.

There's a lot of refactor here.

erikerikson commented 7 years ago

LTS for 4 doesn't end until April 2018.

I am thereby of the opinion that an es6 refactor should be held of until then. @gwsii as the other maintainer here, do you have an opinion to add?

I'm happy to fast track a fix of uri => url. That change was introduced here: https://github.com/shoreditch-ops/artillery-core/commit/48dfbe91d67642fa1c93989acd95af018e6f42c4#diff-08c61297a57965f46e6bf18087f2066b

erikerikson commented 7 years ago

@dougmoscrop between pulls https://github.com/Nordstrom/artillery-plugin-aws-sigv4/pull/10 and https://github.com/Nordstrom/artillery-plugin-aws-sigv4/pull/11 I believe the issues here should be resolved in a compatibiltiy maximizing fashion. Have I missed something that was changed here? Due to the refactor, I had trouble ensuring every change was reflected. Thanks for calling this to attention and offering a solution!

dougmoscrop commented 7 years ago

The only other behavioral change would the async credentials load.

erikerikson commented 7 years ago

Oh, right, thank you for calling that out. We've previously depended upon environment credentials, particularly since we usually use this inside Lambdas but this will increase the usability. Thanks!

erikerikson commented 7 years ago

FYI @dougmoscrop we have published these changes as v0.0.3.

Thank you for bringing attention to this!

dougmoscrop commented 7 years ago

Thanks!

erikerikson commented 7 years ago

Of course. Sorry for the original problem as well as the delay