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 for #74 #75

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

implementation for #74 #75

wants to merge 3 commits into from

Conversation

xbranko
Copy link

@xbranko xbranko commented Jan 15, 2014

This is my first attempt at contributing back via git. Not sure how it all works, but hopefully this will be of help. It worked in my environment. Don't know how to exclude a change (to pom, we hare don't have commons-logging 1.1, but 1.1.1).

@carterpage
Copy link
Member

Good idea. A couple of things:

  1. Don't change existing interface implementations, extend them and add new methods.
  2. Don't change existing tests. Add new ones.

Some day I'd like to extend this to other cloud providers, at which point it will make sense to refactor credentials entirely, but I'm not ready to do that today. I can't break backward compatibility. Please make the changes requested and resubmit.

@carterpage
Copy link
Member

ping

@xbranko
Copy link
Author

xbranko commented Feb 3, 2014

Sorry I got busy with my project once I got it unblocked. I will do as you suggest, and offer it via another pull request.

@xbranko
Copy link
Author

xbranko commented Feb 4, 2014

I tried to do as you asked, but have run into a problem. AmazonAwsSQSConnector that I wanted to extend defines _amazonSQS and _amazonSNS as private final fields that are initialized in its constructor. AWSCredentials object is being passed to their constructors, and at that point the derived class can't offer different implementation, because it is too late.

Did my initial solution break any of your tests or apps? If it didn't, would you reconsider accepting it?
If you know of a better (more elegant, cleaner) way to achieve this, please let me know, or if it is easier feel free to do it.

@carterpage
Copy link
Member

This library is used by lots of developers, so we can't simply change public method signatures. You'd be surprised, but someone is almost certainly using those directly. This change will break their build. Feel free to change the private final fields to non-final, and if needed, protected. Let's see if there's another way to do this.

@xbranko
Copy link
Author

xbranko commented Feb 5, 2014

One way to do it would be to have the SQSConnectorFactory define new getInstance method(s) that take a newly defined credentials object (e.g. of class NevadoCredentials). That way whether there are two, three or more parameters needed, the signature would not need to change. Yet whoever used the current interface would not be adversely affected. Would that work?

@carterpage
Copy link
Member

I think that would work. I want to have a Credentials object at some point
besides. It may take a couple of iterations to get the signatures correct,
but I think that's a good direction.
On Feb 5, 2014 2:06 PM, "xbranko" [email protected] wrote:

One way to do it would be to have the SQSConnectorFactory define new
getInstance method(s) that take a newly defined credentials object (e.g. of
class NevadoCredentials). That way whether there are two, three or more
parameters needed, the signature would not need to change. Yet whoever used
the current interface would not be adversely affected. Would that work?

Reply to this email directly or view it on GitHubhttps://github.com//pull/75#issuecomment-34225103
.

@twicksell
Copy link

@carterpage can we get an update on this issue and PR? I also have a need for session credentials and I'd really like to continue using Nevado.

@carterpage
Copy link
Member

Some work is still required. The pull request as it currently stands is inadmissible because it breaks backwards compatibility. @xbranko suggested an approach, which is sounds great, but it hasn't been submitted. If you would like to take a stab at it, please do. You can deprecate the old methods, but not remove them. I unfortunately don't have the time to implement it any time soon. Thanks for understanding.

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