Skip to content
This repository has been archived by the owner on Dec 5, 2020. It is now read-only.

Added AbstractSystemProperty and 4 system properties implementation. #203

Merged
merged 3 commits into from Mar 29, 2017

Conversation

SherifWaly
Copy link
Contributor

PR for #201

@coveralls
Copy link

coveralls commented Mar 28, 2017

Coverage Status

Coverage decreased (-1.4%) to 85.471% when pulling d603cae on SherifWaly:#201 into 78c41db on opencharles:master.

@SherifWaly
Copy link
Contributor Author

@amihaiemil Please check this commit :D

Copy link
Member

@amihaiemil amihaiemil left a comment

Choose a reason for hiding this comment

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

@SherifWaly Just 2 comments :D

this.name = name;
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

@SherifWaly Indentation to the left here

public StEsEndPoint(final String name) {
super(name);
}

Copy link
Member

Choose a reason for hiding this comment

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

@SherifWaly For this, can you override the read() method, to check if the endpoint ends with "/" or not? If not, then add it. So:

public String read() {
    String endpoint = super.read()
    //... check for '/' here
}

@amihaiemil
Copy link
Member

amihaiemil commented Mar 29, 2017

@SherifWaly As you probably noticed, interfaces Region, SecretKey, EsEndPoint, and AccessKeyId are not really needed, we could just have RtSystemProperty implements SystemProperty. But I decided to let them be since they offer better semantics to the code.

For instance, when instantiating a SignedRequest or an EsHttpRequest, you will clearly see that they expect a Region or an EsEndPoint precisely, rather than a generic SystemProperty. Or, when you'll look at the ctor of AwsCredentialsFromSystem you will clearly see that it expects a SecretKey and an AccessKeyId, rather than two SystemProperty, which you will wonder what represent :D

Also, we will have a few decorators, both for generic SystemProperty, as well as particulars, like EsEndPoint.

Makes sense? What do you think?

Copy link
Member

@amihaiemil amihaiemil left a comment

Choose a reason for hiding this comment

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

@SherifWaly 2 more comments

* @version $Id$
* @since 1.0.0
*/
public final class StAccessKeyId extends AbstractSystemProperty implements SecretKey {
Copy link
Member

Choose a reason for hiding this comment

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

@SherifWaly Here, implements AccessKeyId

* @version $Id$
* @since 1.0.0
*/
public final class StRegion extends AbstractSystemProperty implements SecretKey {
Copy link
Member

Choose a reason for hiding this comment

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

@SherifWaly Here, implements Region

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amihaiemil Sorry for this mistake.

Copy link
Member

@amihaiemil amihaiemil left a comment

Choose a reason for hiding this comment

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

@SherifWaly Looks good! just one more minor thing :D

@Override
public String read() {
String endPoint = super.read();
if(!endPoint.endsWith("\\")) {
Copy link
Member

Choose a reason for hiding this comment

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

@SherifWaly We need "/" (slash), not "\" (backslash) :D Sorry, I might have written it wrong when I let the comment the first time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amihaiemil Ok, I've made a new commit :D

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage decreased (-1.8%) to 85.026% when pulling 8d3d91c on SherifWaly:#201 into 78c41db on opencharles:master.

@SherifWaly
Copy link
Contributor Author

@amihaiemil Please check last commit :)

@amihaiemil
Copy link
Member

@rultor pls merge

@rultor
Copy link
Contributor

rultor commented Mar 29, 2017

@rultor pls merge

@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 44559b4 into opencharles:master Mar 29, 2017
@rultor
Copy link
Contributor

rultor commented Mar 29, 2017

@rultor pls merge

@amihaiemil Done! FYI, the full log is here (took me 3min)

@SherifWaly SherifWaly deleted the #201 branch March 29, 2017 13:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants