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.

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

Provide Graphite scaler #1749

Merged
merged 7 commits into from
Aug 26, 2021

Conversation

shuoliu1
Copy link

@shuoliu1 shuoliu1 commented Apr 21, 2021

Provide a description of what has been changed

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on Provide docs for Graphite scaler keda-docs#425 (if applicable)
  • Changelog has been updated

Fixes #1628

@shuoliu1 shuoliu1 mentioned this pull request Apr 21, 2021
@shuoliu1 shuoliu1 force-pushed the add-graphite-scaler branch 4 times, most recently from 1160fbe to f008368 Compare April 21, 2021 04:14
Signed-off-by: 刘烁 <[email protected]>
Signed-off-by: 刘烁 <[email protected]>
@shuoliu1
Copy link
Author

@tomkerkhove @ahmelsayed add graphite scaler

@tomkerkhove tomkerkhove changed the title add-graphite-scaler Provide Graphite scaler Apr 21, 2021
@tomkerkhove
Copy link
Member

Thanks for the PR! I see you've checked the documentation box but don't see a PR, are you still working on that one?

@shuoliu1
Copy link
Author

Thanks for the PR! I see you've checked the documentation box but don't see a PR, are you still working on that one?

I have submitted it, please review

@shuoliu1
Copy link
Author

I chose some options wrong, it seems that I can't cancel it

@tomkerkhove
Copy link
Member

No problem! Would you mind adding some docs to our doc repo as well please?

@shuoliu1
Copy link
Author

No problem! Would you mind adding some docs to our doc repo as well please?

kedacore/keda-docs#425

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking good. Minor comments:

  1. Could you please update Changelog? (Unreleased/New section)
  2. Could you please implement e2e tests? https://github.com/kedacore/keda/tree/main/tests
  3. I am curious about authentication, I see that none is used for this scaler. Do you think that it does make sense to add some option? (not sure what is the common pattern for Graphite)

@zroubalik zroubalik self-requested a review May 4, 2021 16:38
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

  1. Could you please update Changelog? (Unreleased/New section)
  2. Could you please implement e2e tests? https://github.com/kedacore/keda/tree/main/tests
  3. I am curious about authentication, I see that none is used for this scaler. Do you think that it does make sense to add some option? (not sure what is the common pattern for Graphite)

@shuoliu1
Copy link
Author

  1. Could you please update Changelog? (Unreleased/New section)
  2. Could you please implement e2e tests? https://github.com/kedacore/keda/tree/main/tests
  3. I am curious about authentication, I see that none is used for this scaler. Do you think that it does make sense to add some option? (not sure what is the common pattern for Graphite)

1.update Changelog Done
3.basic authentication add Done

@shuoliu1 shuoliu1 requested a review from zroubalik May 10, 2021 02:44
fix
Signed-off-by: 刘烁 <[email protected]>
@zroubalik
Copy link
Member

  1. Could you please update Changelog? (Unreleased/New section)
  2. Could you please implement e2e tests? https://github.com/kedacore/keda/tree/main/tests
  3. I am curious about authentication, I see that none is used for this scaler. Do you think that it does make sense to add some option? (not sure what is the common pattern for Graphite)

1.update Changelog Done
3.basic authentication add Done

Thanks, please open another PR for the documentation of basic authentication parameters. And what about the e2e tests? Do you plan to add them?

@shuoliu1
Copy link
Author

  1. Could you please update Changelog? (Unreleased/New section)
  2. Could you please implement e2e tests? https://github.com/kedacore/keda/tree/main/tests
  3. I am curious about authentication, I see that none is used for this scaler. Do you think that it does make sense to add some option? (not sure what is the common pattern for Graphite)

1.update Changelog Done
3.basic authentication add Done

Thanks, please open another PR for the documentation of basic authentication parameters. And what about the e2e tests? Do you plan to add them?
documentation pr kedacore/keda-docs#446
e2e test There's no time for that....

@tomkerkhove
Copy link
Member

e2e test There's no time for that....

Unfortunately this is essential before we can merge it so I see a few options:

  1. Tests are added to this PR by you and we can merge it
  2. You do not have time, but do a reach out to our Slack channel and somebody helps you
  3. Close this PR and start as an external scaler first

The reason why it's essential is that we need to guarantee quality but if we don't have the tests then we can't do that. I hope you understand?

@shuoliu1
Copy link
Author

The reason why it's essential is that we need to guarantee quality but if we don't have the tests then we can't do that. I hope you understand?

thank you,add tests I will do it at free time

@tomkerkhove
Copy link
Member

Thank you!

@tomkerkhove
Copy link
Member

Any update on this @shuoliu1 ?

@tomkerkhove
Copy link
Member

Just checking in if you'll have time to finish this PR @shuoliu1?

@nmische
Copy link

nmische commented Aug 25, 2021

I noticed this scaler is listed in the 2.4 docs, but I don't see evidence of that it is in the 2.4 source. Is this available in 2.4?

tomkerkhove added a commit to tomkerkhove/keda-docs that referenced this pull request Aug 25, 2021
@tomkerkhove
Copy link
Member

No and I have merged it by accident and forgot to remove it.

kedacore/keda-docs#518

Are you willing to complete this PR?

@nmische
Copy link

nmische commented Aug 25, 2021

@tomkerkhove: I am definitely interested in this capability. Let me see if I can find some cycles to dive in over the next week or so.

@tomkerkhove
Copy link
Member

Awesome, thank you! Do you want to extend this PR or start fresh? Otherwise I could merge it in a WIP branch on the repo for you.

zroubalik pushed a commit to kedacore/keda-docs that referenced this pull request Aug 26, 2021
@nmische
Copy link

nmische commented Aug 26, 2021

I will likely extend this PR. Thanks.

@tomkerkhove tomkerkhove changed the base branch from main to graphite-wip August 26, 2021 13:52
@tomkerkhove tomkerkhove merged commit f795cbc into kedacore:graphite-wip Aug 26, 2021
@tomkerkhove
Copy link
Member

Pushed it to graphite-wip as-is - Thank you @nmische!

@bpinske
Copy link
Contributor

bpinske commented Sep 6, 2021

Dang someone beat me to it. I was going to pick this up.

bpinske pushed a commit to bpinske/keda that referenced this pull request Sep 6, 2021
Co-authored-by: 刘烁 <[email protected]>
Co-authored-by: Tom Kerkhove <[email protected]>
bpinske pushed a commit to bpinske/keda that referenced this pull request Sep 6, 2021
Co-authored-by: 刘烁 <[email protected]>
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: Brandon Pinske <[email protected]>
@bpinske
Copy link
Contributor

bpinske commented Sep 6, 2021

Just kidding. I did it anyway.

#2092

bpinske pushed a commit to bpinske/keda that referenced this pull request Sep 6, 2021
Co-authored-by: 刘烁 <[email protected]>
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: Brandon Pinske <[email protected]>
@tomkerkhove
Copy link
Member

Thanks a ton @bpinske!

bpinske pushed a commit to bpinske/keda that referenced this pull request Sep 13, 2021
Co-authored-by: 刘烁 <[email protected]>
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: Brandon Pinske <[email protected]>
bpinske pushed a commit to bpinske/keda that referenced this pull request Sep 13, 2021
Co-authored-by: 刘烁 <[email protected]>
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: shuoliu1 [email protected]
bpinske pushed a commit to bpinske/keda that referenced this pull request Sep 13, 2021
Co-authored-by: 刘烁 <[email protected]>
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: shuoliu1 [email protected]
Signed-off-by: Brandon Pinske <[email protected]>
Signed-off-by: Brandon Pinske <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

graphite 支持
5 participants