-
Notifications
You must be signed in to change notification settings - Fork 48
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
base: master
Are you sure you want to change the base?
Conversation
Good idea. A couple of things:
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. |
ping |
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. |
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? |
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. |
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? |
I think that would work. I want to have a Credentials object at some point
|
@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. |
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. |
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).