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

Implementation of GCP Transport #6

Merged
merged 39 commits into from
Apr 23, 2020
Merged

Conversation

aksrajvanshi
Copy link
Contributor

@DImuthuUpe @smarru

We have created the GCP Transport implementation along with a basic API Gateway implementation to initiate transfers and check the status of the transfers. You need to add the path of Google Cloud Project service account credential JSON into secrets.json for Secret Service.

Your feedback is most welcome.

Copy link
Contributor

@DImuthuUpe DImuthuUpe left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and it's really good to see that you have captured transport integration properly. I need you to cleanup the PR a little bit as there are some unrelated files and changes into the config files. As a major design change, I think that secret json should be serialized into the secret object itself rather than loading from a file path. Good job. Just few corrections needed

api-gateway/.gitignore Outdated Show resolved Hide resolved
api-gateway/.mvn/wrapper/MavenWrapperDownloader.java Outdated Show resolved Hide resolved
api-gateway/.mvn/wrapper/maven-wrapper.properties Outdated Show resolved Hide resolved
api-gateway/mvnw Outdated Show resolved Hide resolved
api-gateway/mvnw.cmd Outdated Show resolved Hide resolved
@pokearu
Copy link
Contributor

pokearu commented Apr 22, 2020

We have implemented the suggested changes.
Please check and let us know.

transport/gcp-transport/pom.xml Show resolved Hide resolved

HttpTransport transport = GoogleNetHttpTransport.newTrustedTransport();
JsonFactory jsonFactory = new JacksonFactory();
String jsonString = gcsSecret.getJsonCredentialsFilePath();
Copy link
Contributor

Choose a reason for hiding this comment

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

Good. Please change the property name to jsonFileCredentialsFilePath => credentialJson and refer to my comments given to file based secret backend

@pokearu
Copy link
Contributor

pokearu commented Apr 22, 2020

We have implemented the requested changes. Please do check and let us know. 👍

Copy link
Contributor

@DImuthuUpe DImuthuUpe left a comment

Choose a reason for hiding this comment

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

All looks good to me except there is a minor suggestion that I have added and missing license headers of all new files. Because this is a apache 2.0 licensed code base, you should add license header for all java, pom files

@pokearu
Copy link
Contributor

pokearu commented Apr 23, 2020

@DImuthuUpe Okay we will add the license headers 👍

Sorry, but i am a bit unclear on what are the minor suggestions you are referring to? Is it about the indentation or the content type as null?

@DImuthuUpe
Copy link
Contributor

@DImuthuUpe Okay we will add the license headers 👍

Sorry, but i am a bit unclear on what are the minor suggestions you are referring to? Is it about the indentation or the content type as null?

It's the indentation issue. I saw that you have changed the content type to null

@aksrajvanshi
Copy link
Contributor Author

We have changed the indentation issue and added the license headers. Can you please look into the changes once again?

@DImuthuUpe
Copy link
Contributor

All look good to me. Great work. I'm going to merge this

@DImuthuUpe DImuthuUpe merged commit 0c2b19d into apache:master Apr 23, 2020
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.

4 participants