-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Move to getParameterFromConfigV2 for input, excluding auth #5639
Move to getParameterFromConfigV2 for input, excluding auth #5639
Conversation
Signed-off-by: Rob Pickerill <r.pickerill@gmail.com>
cb18e78
to
6bce335
Compare
Signed-off-by: Rob Pickerill <r.pickerill@gmail.com>
668ffda
to
618447d
Compare
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! Nice improvement!
PTAL @dttung2905
/run-e2e cloudwatch |
Sorry I will try to take a look tomorrow morning😬 |
@robpickerill , |
Signed-off-by: Rob Pickerill <r.pickerill@gmail.com>
74775bb
to
1b6a871
Compare
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.
Hi @robpickerill,
First of all, thank you so much for putting in effort into making this PR. Generally, its LGTM. Just one minor point that we have another PR #5319 that changes how getParameterFromConfigV2
works.
Is this PR blocking the other PR #5635 ? We can either:
(1) wait for #5319 to be merged first , then this PR and #5635 or
(2) we move forward with this current implementation of getParameterFromConfigV2
in the main
branch. I will rebase yours into #5319
WDYT? @JorTurFer I'm in favour of (1) and I will try my best to merge my PR. Its almost done tbh :D
Thanks @dttung2905, by all means take option 1 for me, feel free to tag me in the updates for when you finalize the implementation and I'll refactor this PR as needed. Also let me know if I can do anything to help, thanks again! |
Let's wait with this and then rebase it if you agree |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
@dttung2905 we can close this, right? |
Yes we can |
Thanks for the work anyway! |
Moves the AWS CloudWatch scaler to use getParameterFromConfigV2 for input.
Context is from a conversation started here: #5635 (comment)
There is one TODO here, which is to move authorization over. I'll present that in a separate PR to avoid a large PR landing, and I can spend more time to read through the authorization code.
Checklist