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

Fix loading Modules #95

Merged
merged 14 commits into from
Apr 7, 2018
Merged

Fix loading Modules #95

merged 14 commits into from
Apr 7, 2018

Conversation

ipapapa
Copy link
Contributor

@ipapapa ipapapa commented Apr 5, 2018

Copy link
Contributor

@amcp amcp left a comment

Choose a reason for hiding this comment

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

I have some minor comments

@@ -96,4 +96,11 @@
@DefaultValue("xxx:8111")
String getDaxEndpoint();


@PropertyName(name = "AWSAccessKey")
Copy link
Contributor

Choose a reason for hiding this comment

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

Using application.properties means that the credentials have to go into a file curated by this project. Could we use DefaultCredentialProviderChain, so that if run on managed compute, credentials get pulled from the instance profile, and otherwise get pulled from environment variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, we are also picking up the credentials from the instance profile. This seems to be breaking our internal deployment as well. I will into how to use DefaultCredentialProviderChain.

Copy link
Contributor

Choose a reason for hiding this comment

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

lets make an issue to fix credentials later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been using the plugin for some time now and this breaks our internal bindings with AWSCredentials. To move forward with the rest of your work, I will merge it and will revisit on Monday.

@@ -12,6 +12,9 @@ dependencies {
compile project(':ndbench-es-plugins')
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: I would opt to order this list of plugins in alphabetical order, so it is easier to spot missing dependencies in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@ipapapa
Copy link
Contributor Author

ipapapa commented Apr 6, 2018

Rebased with master for #96 and #99

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

Successfully merging this pull request may close these issues.

4 participants