-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Fix for #9135 #9156
Fix for #9135 #9156
Conversation
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.
Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla
!signed-cla |
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.
Provided that this solves the issue that other users were having, which it sounds like it does, this makes sense to me.
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.
Looks like new artifacts were built from this PR. Get them here!
Artifact URLs
What a relief :D.... I had opened to create this PR myself....if there wasn't movement :) |
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.
Thanks for tackling this so quickly! I have some comments in the code. Exporting of the discovery part and the channel seems unnecessary, so please revert if you don't have any good reason of doing it. :-)
@vipinvkmenon can you please test if this fixes your problem and add a comment!?
Tried another configuration:
got the output:
Indicating the plugin works for |
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.
It seems like the code is not yet reflecting the resolved
state in the comment. Did you maybe forget to push your latest changes?
Sorry, looks like I forgot to push changes. Pushed just now. |
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.
Looks like new artifacts were built from this PR. Get them here!
Artifact URLs
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.
Almost there. Just a few minor things, see comments in the code...
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.
Looks like new artifacts were built from this PR. Get them here!
Artifact URLs
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.
Code looks good. @i-prudnikov the bot thinks you are missing the CLA, if you already signed it, please write a comment with !signed-cla
. Let me know if there are problems...
Hello, I've already put the comment with |
|
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.
🤝 ✅ CLA has been signed. Thank you!
@i-prudnikov sorry about the CLA bot confusion, there was an issue that should be resolved now. |
Sure, NP! |
@i-prudnikov in aliyuncms |
@vipinvkmenon , I think that the source of confusion is this page: https://help.aliyun.com/document_detail/28619.html?spm=a2c4g.11186623.6.690.1938ad41wg8QSq - here they use term |
@vipinvkmenon can we please stay on-topic here! ;-P There is a slack channel to discuss this kind of things... :-) |
From the test perspective the PR works :) |
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.
lgtm! @i-prudnikov once you've resolved the conflicts, I will merge this pr. Thanks!
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Required for all PRs:
resolves #9135
According to symptoms, it looks like Alicloud changes the API behaviour, this fix is to address more strict metrics querying specifying region.
So we need to specify region for metric gathering. As we already have discovery region, I've decided to obsolete
discovery_region
attribute in favour ofregions
attribute that specify both: metrics and discovery scope.Old configs are still supported and deprecation warning is printed if discovery_region is used.
In case only
discovery_regions
is specified thenregions
is populated from it. If both specified thendiscovery_regions
is omitted.