-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Support QueueUrl Plugin override #2148
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
Conversation
CI passing the 2.1 failing is transient issue as it's succeeding in the other run |
I wonder if we could do something like the following, rather than having to disable the plugin through a flag. In the queue_url plugin, we check if an endpoint is provided by the end user. If not, then go ahead and update the endpoint and region based on the queue_url. If yes, then do not make those changes. That would make it easier to use custom endpoints, rather than having to remember to pass in |
@hhk1989 Thanks for the feedback! Appreciate that, personally thought about that too, was hesitate to do so because it could potentially breaking people providing a customized endpoint but depending on current overwrite, it's hard to tell who might be replying on this behavior. Adding an option would make it safer for people who are certain that they want to enable this behavior. In the next major version (where breaking changes are allowed), we probably should consider making this override turned off by default when customized endpoint is provided. Thoughts? |
That's a fair point, it's prudent to not break existing workflows. However, could we also keep this consistent for the Java SDK ? As in for now, we introduce the |
@hhk1989 Sounds good! I will sync with Java team to make sure we are on the same page |
class Handler < Seahorse::Client::Handler | ||
|
||
def call(context) | ||
if queue_url = context.params[:queue_url] | ||
disable = !!context.config.disable_queue_url_override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid use of double negation (as per Rubocop). Use .nil?
it 'does not override endpoint and region when disabled' do | ||
endpoint = 'https://vpc-123234.sqs.us-west-2.vpc.amazonaws.com' | ||
url = 'https://sqs.us-east-1.amazonaws.com/1234567890/demo' | ||
client = Client.new(stub_responses: true, endpoint: endpoint, disable_queue_url_override: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for 80 chars!
closing, will create a separate PR for tracking as the issue is queue url plugin is parsing out the wrong region from vpc endpoint queue |
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Addressing #2114
will disable endpoint and region override from the
:queue _url
providedNote:
Pending on sync with Java team