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

Configurable Root url #82

Merged
merged 2 commits into from
Nov 29, 2017
Merged

Configurable Root url #82

merged 2 commits into from
Nov 29, 2017

Conversation

billoneil
Copy link
Collaborator

I actually need this for security purposes. Also bumped versions of dependencies.

private final String key;
private final String rootUrl;

public MandrillExportsApi(final String key, final String url) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to modify the original constructor but since this is a public class with a public constructor that would break backwards compatibility which is why I chose overloading.

@@ -22,6 +22,8 @@
* @since Mar 17, 2013
*/
public class MandrillApi {
public static final String rootUrl = "https://mandrillapp.com/api/1.0/";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is public because we need to default it here as well as in the other classes for backwards compatibility reasons. See below comment.

Copy link
Owner

Choose a reason for hiding this comment

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

ok

@billoneil
Copy link
Collaborator Author

The testContent01 test was failing for me because I was using an unverified sending domain with my tests. Could you double check that test if possible @rschreijer.

<version>1.3.2</version>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.5</version> <!-- 2.5 is the last version to support java 1.6 -->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this might be able to be switched to a test only dependency. It could also be a runtime dependency of the HttpClient though so I didn't change it.

Copy link
Owner

@rschreijer rschreijer left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -22,6 +22,8 @@
* @since Mar 17, 2013
*/
public class MandrillApi {
public static final String rootUrl = "https://mandrillapp.com/api/1.0/";
Copy link
Owner

Choose a reason for hiding this comment

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

ok

@billoneil billoneil merged commit 170cd34 into master Nov 29, 2017
@billoneil billoneil deleted the configurable-url branch November 29, 2017 20:16
@thberger
Copy link

Thanks for the library! Any chances that version 0.0.8 including this fix will be released soon?

@rschreijer
Copy link
Owner

it's in the pipeline.

@thberger
Copy link

thberger commented Dec 1, 2017

@rschreijer: Thank you for the release, really helps us a lot.

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.

3 participants