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

Add ActiveMQ Classic Scaler #2121

Closed

Conversation

melisatanrverdi
Copy link
Contributor

@melisatanrverdi melisatanrverdi commented Sep 27, 2021

Signed-off-by: melisatanrverdi melisatanrverdi@gmail.com

Add ActiveMQ Classic Scaler

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 (repo) (if applicable)
  • Changelog has been updated

Relates to #2120

Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
@melisatanrverdi melisatanrverdi changed the title signed-off-by: melisatanrverdi melisatanrverdi@gmail.com Add ActiveMQ Classic Scaler Sep 27, 2021
@tomkerkhove
Copy link
Member

Thank you for your PR!

@tomkerkhove
Copy link
Member

Just for my reference as an ActiveMQ newbie - What's the difference between classic and Artemis that we support today?

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.

Thanks for this PR! I have a couple of questions/remarks:

  1. I wonder whether AciveMQ Classic is the correct name for the scaler, I mean the Classic part, if I am not mistaken there is no such project/product. I think we can name this scaler just ActiveMQ, what do you think?
  2. please fix the DCO https://github.com/kedacore/keda/blob/main/CONTRIBUTING.md#developer-certificate-of-origin-signing-your-work
  3. please format the code and fix all the linter related problems: https://github.com/kedacore/keda/pull/2121/checks?check_run_id=3726021740
  4. please add unit tests
  5. please add e2e tests: https://github.com/kedacore/keda/tree/main/tests

@melisatanrverdi
Copy link
Contributor Author

Just for my reference as an ActiveMQ newbie - What's the difference between classic and Artemis that we support today?

I am also newbie and this helped me to understand the difference as it explains quite simply: https://activemq.apache.org/components/artemis/migration-documentation/key-differences.html

@melisatanrverdi
Copy link
Contributor Author

Thanks for this PR! I have a couple of questions/remarks:

1. I wonder whether `AciveMQ Classic` is the correct name for the scaler, I mean the `Classic` part, if I am not mistaken there is no such project/product. I think we can name this scaler just `ActiveMQ`, what do you think?

2. please fix the DCO https://github.com/kedacore/keda/blob/main/CONTRIBUTING.md#developer-certificate-of-origin-signing-your-work

3. please format the code and fix all the linter related problems: https://github.com/kedacore/keda/pull/2121/checks?check_run_id=3726021740

4. please add unit tests

5. please add e2e tests: https://github.com/kedacore/keda/tree/main/tests

Yes you are right, I changed the name as ActiveMQ. I am working on tests.

Copy link
Contributor

@arschles arschles 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 addressing my comments. My only question left is about unifying the artemis and classic scaler codebases

@melisatanrverdi
Copy link
Contributor Author

My branch is behind with a ton of commits so I am closing this PR. I will open a new one.

@arschles
Copy link
Contributor

@melisatanrverdi sounds good 👍

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.

4 participants