NIFI-5022 InvokeAWSGatewayApi processor #2588

ottobackwards · 2018-03-27T22:22:15Z

This processor's intention is to provide the same interface as InvokeHttp for AWS Gateway Web Api hosted endpoints.

To that end, all of the applicable InvokeHttp tests have been refactored for this processor as well.

Testing will require a hosted api ( Amazon does not have a 'local' version ).

Creating a sample api can be used to stand up an api.

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

ottobackwards · 2018-03-27T22:25:39Z

Notes:

I am still validating against aws service/petstore
I have implemented proxy user/password in these classes, there is another PR that adds it to the base, but I didn't want to wait and didn't want to expand scope. I did however want all the tests from InvokeHttp to run. This will have to be addressed

ottobackwards · 2018-04-12T10:47:36Z

@MikeThomsen @pvillard31 @mattyb149 @mans2singh Anyone up for a code review?

ottobackwards · 2018-04-12T11:49:57Z

https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-create-api-from-example.html

If you can do this you can test with the pet store sample

pvillard31 · 2018-04-12T12:12:30Z

Hey @ottobackwards - I'd love to have a look but I won't have cycles to do a proper review and testing before end of next week at best.

ottobackwards · 2018-04-28T16:38:01Z

@zenfenan any chance for a review?

zenfenan · 2018-04-28T16:42:53Z

@ottobackwards I would really love to do it but I'm occupied until next week. If no one takes a look by that time, I'll definitely do that.

ottobackwards · 2018-05-03T14:02:14Z

It would be better if we would just use RPGreen's maven dep instead off bringing in the classes. I have a pr against his repo to upgrade to the same aws package we use. When that is taken and a release is cut, I'll move over to that. I think we should still review however, in the event that he doesn't take that pr and we have to keep the code.

ottobackwards · 2018-05-03T14:02:23Z

rpgreen/apigateway-generic-java-sdk#8

mattyb149 · 2018-05-03T14:09:41Z

Sounds good to me, especially if that project is active, I'd like to keep our dependencies as modules rather than code if possible.

ottobackwards · 2018-05-18T11:07:36Z

@mattyb149 no word on that pr. @jvwing any chance you may be able to review?

ottobackwards · 2018-05-21T16:50:42Z

bump, any takers?

ottobackwards · 2018-06-05T17:12:26Z

I can provide the reviewers with access to my PETSTORE endpoint for testing and we can work out how not to overload it, if that helps get this going

jzonthemtn · 2018-06-11T11:13:16Z

...ws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/wag/client/Validate.java

+ */
+package org.apache.nifi.processors.aws.wag.client;
+
+import com.amazonaws.util.StringUtils;


Just wondering if you meant this StringUtils. I do this a lot when I really want StringUtils from commons lang.

This is part of the code from an external library, and I believe this is intentional.

Ok, thanks!

jzonthemtn · 2018-06-11T11:23:51Z

...cessors/src/main/java/org/apache/nifi/processors/aws/wag/client/GenericApiGatewayClient.java

+            headers = new HashMap<>();
+        }
+        if (apiKey != null) {
+            final Map<String, String> headersWithApiKey = new HashMap<>();


Is it possible to just add the API key to headers without making a new map and then always return headers;?

jzonthemtn · 2018-06-11T12:22:06Z

@ottobackwards Haven't tested it but code-wise it makes sense.

joewitt · 2018-06-11T13:04:03Z

@ottobackwards I would be fine merging this in without running additional testing like you've already done. What we cannot have though are the ASF headers on source code we copied from elsewhere. You did a great job of adding it to the NOTICE files but we cannot add the ASF header. Also, why can't we just use the library as it exists as a binary dependency? Are there some critical changes or something?

ottobackwards · 2018-06-11T13:13:00Z

@joewitt
I added the headers because the code was already apache lic. in the project, I thought that would be the proper thing to do. I ( possibly or probably wrongly ) thought that if you put the lic. in the project you should lic. the files with headers.

As for taking the library vs. including the code.

I took that code because I was not sure if the library would be updated quickly enough if there where changes in the AWS library versions in nifi or other changes required ( such as my need to add support for query parameters ). Also, there is not a lot of code to work with. @mattyb149 and I talked it out and I have done a pr to the project for a version bump that would allow us to use the library, but there has been no response to that pr since May 3rd when I posted it ( as I had feared at the start ).

Currently I think the options are ( in no order )

The PR goes with the code as it is, with a notice to move to the library if and when it is updated to a comparable version of aws
I do a proper fork into my github organization with the required version and publish ( currently I just do bintray and center )
This PR is shelved until the project takes the PR

Obviously I would prefer not to shelve this.
Thoughts?

Should I remove the headers?

ottobackwards · 2018-06-11T13:14:54Z

Do I need to add ratcheck exceptions if I remove the headers?

joewitt · 2018-06-11T13:42:08Z

@ottobackwards this describes how to handle code contributed to the ASF
https://www.apache.org/legal/src-headers.html#headers
In short that is when you add the header.

This describes how to handle code not contributed to but rather pulled into ASF projects (third party works):
https://www.apache.org/legal/src-headers.html#3party
In short dont add the header.

So yes please remove the header from all files which are sourced externally. If you were simply pulling in a code snippet/etc.. then it would be ok to just cite it like you did and have the header but this case is pretty simple.

Anyway, easy fix, just remove header and yes add RAT exclusions for each and put a comment explaining in case someone flags it later.

Your comments about them not doing a release with the PR you did are fine. It is ok to pull source in this case just obviously preferable to avoid it where possible. So all good. The other rub here is the author doesn't tag releases which helps us in terms of provenance. Can you please reference a specific commit in the git repo you're pulling from in the notice entry. Not strictly necessary but safer in terms of 'it was ALv2 licensed when i pulled this'.

I'll try to look again when that is done

ottobackwards · 2018-06-11T13:51:29Z

Thanks @joewitt for clearing that up. I'll get right to it.

- removed apache headers from 3rd party files per https://www.apache.org/legal/src-headers.html#3party - referenced commit/repo in notice file - created rat configuration to account for files without headers

ottobackwards · 2018-06-11T14:31:36Z

@joewitt requested changes are in

joewitt · 2018-06-11T14:48:27Z

full clean build w/contrib check passes.
review of code all seems ok and no new dep issues
license and notice updates correct/header changes.

My review was more legal/structural than functional. If there are functional improvements needed we can do them over time as this is safely isolated from impacting other items. So +1 merging to master.

ottobackwards · 2018-06-11T14:58:39Z

Thanks everybody!

ottobackwards changed the title ~~NIFI-5022 InvokeAWSGatewayApi processor~~ NIFI-5022 [WIP]InvokeAWSGatewayApi processor Mar 27, 2018

ottobackwards mentioned this pull request Mar 29, 2018

Handle null response body and support Parameters rpgreen/apigateway-generic-java-sdk#7

Merged

ottobackwards changed the title ~~NIFI-5022 [WIP]InvokeAWSGatewayApi processor~~ NIFI-5022 InvokeAWSGatewayApi processor Apr 4, 2018

ottobackwards mentioned this pull request Apr 9, 2018

NIFI-4196 - Expose AWS proxy authentication settings #2016

Closed

11 tasks

ottobackwards force-pushed the aws-web-api branch from 8ec58d4 to f7bed64 Compare April 12, 2018 10:51

ottobackwards force-pushed the aws-web-api branch from 451949c to 3cbdab7 Compare April 23, 2018 12:06

ottobackwards force-pushed the aws-web-api branch 3 times, most recently from 15b856d to 9cd1798 Compare May 17, 2018 17:25

NIFI-5022 InvokeAWSGatewayApi processor

cb07b4c

ottobackwards force-pushed the aws-web-api branch from 480f393 to cb07b4c Compare May 31, 2018 12:21

Merge branch 'master' of https://github.com/apache/nifi into aws-web-api

57dc6f8

jzonthemtn reviewed Jun 11, 2018

View reviewed changes

per review, simplify header building and remove new map

65639bb

Per review:

b51bfc3

- removed apache headers from 3rd party files per https://www.apache.org/legal/src-headers.html#3party - referenced commit/repo in notice file - created rat configuration to account for files without headers

asfgit closed this in 946c2fe Jun 11, 2018

NIFI-5022 InvokeAWSGatewayApi processor #2588

NIFI-5022 InvokeAWSGatewayApi processor #2588

Uh oh!

Conversation

ottobackwards commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

ottobackwards commented Mar 27, 2018

Uh oh!

ottobackwards commented Apr 12, 2018

Uh oh!

ottobackwards commented Apr 12, 2018

Uh oh!

pvillard31 commented Apr 12, 2018

Uh oh!

ottobackwards commented Apr 28, 2018

Uh oh!

zenfenan commented Apr 28, 2018

Uh oh!

ottobackwards commented May 3, 2018

Uh oh!

ottobackwards commented May 3, 2018

Uh oh!

mattyb149 commented May 3, 2018

Uh oh!

ottobackwards commented May 18, 2018

Uh oh!

ottobackwards commented May 21, 2018

Uh oh!

ottobackwards commented Jun 5, 2018

Uh oh!

jzonthemtn Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

ottobackwards Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

jzonthemtn Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

jzonthemtn Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

ottobackwards Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

jzonthemtn commented Jun 11, 2018

Uh oh!

joewitt commented Jun 11, 2018

Uh oh!

ottobackwards commented Jun 11, 2018

Uh oh!

ottobackwards commented Jun 11, 2018

Uh oh!

joewitt commented Jun 11, 2018

Uh oh!

ottobackwards commented Jun 11, 2018

Uh oh!

ottobackwards commented Jun 11, 2018

Uh oh!

joewitt commented Jun 11, 2018

Uh oh!

ottobackwards commented Jun 11, 2018

Uh oh!

Uh oh!

ottobackwards commented Mar 27, 2018 •

edited

Loading