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.

Sign up for GitHub

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

Jump to bottom

add kinesis stream scaler content #61

Merged
merged 1 commit into from
Dec 24, 2019
Merged

add kinesis stream scaler content #61

merged 1 commit into from
Dec 24, 2019

Conversation

msfuko
Copy link
Contributor

@msfuko msfuko commented Dec 23, 2019

The doc for kedacore/keda#526.

@msfuko msfuko mentioned this pull request Dec 23, 2019
Merged
tomkerkhove
tomkerkhove suggested changes Dec 24, 2019
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting the PR! Always happy to see a new scaler come to life.

I've added a few comments to improve the docs, can you please have a look?

content/scalers/aws-kinesis.md Outdated Show resolved Hide resolved
content/scalers/aws-kinesis.md Outdated Show resolved Hide resolved
content/scalers/aws-kinesis.md Outdated Show resolved Hide resolved
content/scalers/aws-kinesis.md Outdated Show resolved Hide resolved
content/scalers/aws-kinesis.md Outdated Show resolved Hide resolved
content/scalers/aws-kinesis.md Outdated
- `awsAccessKeyID` - Id of the user
- `awsSecretAccessKey` - Access key for the user to authenticate with

The user will need access to read data from AWS Cloudwatch.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still applicable to get the metrics via Cloudwatch or is this a copy/paste mistake?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you think here @msfuko and should be good to go

Copy link
Contributor Author

@msfuko msfuko Dec 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I removed the whole sentence. to be more precise, I added "The user will need DescribeStreamSummary IAM permission policy to read data from AWS Kinesis Streams." here. hope this looks good.
https://github.com/kedacore/keda-docs/pull/61/files#diff-664a100428eb94713fde5e3721428185R51

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

content/scalers/aws-kinesis.md Outdated Show resolved Hide resolved
@msfuko msfuko force-pushed the kinesis_stream_scaler branch from cf352d9 to 2cbe9f5 Compare December 24, 2019 12:36
@msfuko
Copy link
Contributor Author

msfuko commented Dec 24, 2019

Hey @tomkerkhove Thanks for the code review! I've updated accordingly.
The change isn't huge so I merge them into a single commit. Please let me know if I should just submit the separate commit.
Thanks again for your time!

tomkerkhove
tomkerkhove approved these changes Dec 24, 2019
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@msfuko msfuko force-pushed the kinesis_stream_scaler branch from 2cbe9f5 to ec73f25 Compare December 24, 2019 16:43
@msfuko msfuko force-pushed the kinesis_stream_scaler branch from ec73f25 to 8091875 Compare December 24, 2019 16:47
@tomkerkhove tomkerkhove merged commit 1479c67 into kedacore:master Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomkerkhove tomkerkhove tomkerkhove approved these changes

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

@jeffhollan jeffhollan Awaiting requested review from jeffhollan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@msfuko @tomkerkhove