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

Support profiling for keda components #5091

Merged
merged 8 commits into from
Oct 31, 2023
Merged

Conversation

yuvalweber
Copy link
Contributor

@yuvalweber yuvalweber commented Oct 17, 2023

Provide a description of what has been changed

Checklist

Fixes #

Relates to #4789

Signed-off-by: yuval weber <yuval199985@gmail.com>
@yuvalweber yuvalweber requested a review from a team as a code owner October 17, 2023 14:03
@github-actions
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

Signed-off-by: yuval weber <yuval199985@gmail.com>
Signed-off-by: yuval weber <yuval199985@gmail.com>
Signed-off-by: yuval weber <yuval199985@gmail.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: yuval weber <yuval199985@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented Oct 18, 2023

/run-e2e internal
Update: You can check the progress here

@yuvalweber
Copy link
Contributor Author

Hey @zroubalik can you please inform me what is missing in all of the PRs I opened (kedacore/charts#549, kedacore/keda-docs#1250, #5091 ) in order to merge everything?

@tomkerkhove
Copy link
Member

Hey @zroubalik can you please inform me what is missing in all of the PRs I opened (kedacore/charts#549, kedacore/keda-docs#1250, #5091 ) in order to merge everything?

@yuvalweber The e2e tests are failing, can you please check?

/run-e2e internal Update: You can check the progress here

@yuvalweber
Copy link
Contributor Author

Hey @tomkerkhove can you help me a bit?
I looked at the errors and I see things failing because it's trying to create resources on namespaces that being terminated or something with rate limit, can you please help me figure out if it's related to my change (cause it shouldn't)?

@yuvalweber
Copy link
Contributor Author

Hey @tomkerkhove would love for also update on this

@zroubalik
Copy link
Member

zroubalik commented Oct 19, 2023

/run-e2e internal
Update: You can check the progress here

@zroubalik
Copy link
Member

Hey @tomkerkhove can you help me a bit? I looked at the errors and I see things failing because it's trying to create resources on namespaces that being terminated or something with rate limit, can you please help me figure out if it's related to my change (cause it shouldn't)?

Env problem, the re-run passed 🚀

@yuvalweber
Copy link
Contributor Author

Hey @zroubalik do I need to write test?
Sorry i'm just not familiar with writing tests in golang and not sure if necessary in this situation

@zroubalik
Copy link
Member

Hey @zroubalik do I need to write test? Sorry i'm just not familiar with writing tests in golang and not sure if necessary in this situation

@yuvalweber I don't think tests are needed for this kind of feature. On the other hand a short blogpost on how you used this profiler would be awesome 😃

@yuvalweber
Copy link
Contributor Author

Would love to do it 😄 .
Can you share example of blogpost so I could see the format and know how to copy it?
I could share the example related to the issue I had before #4687

@zroubalik
Copy link
Member

Would love to do it 😄 . Can you share example of blogpost so I could see the format and know how to copy it? I could share the example related to the issue I had before #4687

DM me on slack :)

@yuvalweber
Copy link
Contributor Author

Hey @zroubalik need you to rerun the e2e tests.
I wanted to merge PR and rebase but accidentally just updated by branch which triggered this new requirement for this test

@yuvalweber
Copy link
Contributor Author

Hey @zroubalik can you just help me understand why it won't let me merge this?

@zroubalik
Copy link
Member

zroubalik commented Oct 20, 2023

Hey @zroubalik can you just help me understand why it won't let me merge this?

Only maitiainers can merge a PR, we usually do that once all PRs (feature/charts/docs) are ready.

@yuvalweber
Copy link
Contributor Author

Alright so when @tomkerkhove will finish reviewing my changes I'll be happy to merge.
Meanwhile I'll work on the blogpost

@zroubalik
Copy link
Member

Alright so when @tomkerkhove will finish reviewing my changes I'll be happy to merge. Meanwhile I'll work on the blogpost

You don't have to do anything on the PRs (only additional fixes requested by reviews). We will handle the rest

@yuvalweber
Copy link
Contributor Author

Alright so when @tomkerkhove will finish reviewing my changes I'll be happy to merge. Meanwhile I'll work on the blogpost

You don't have to do anything on the PRs (only additional fixes requested by reviews). We will handle the rest

I think I addressed to all of the requests you guys reviewed but if I missed something please let me know 😄

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
@zroubalik zroubalik merged commit 4cbc74e into kedacore:main Oct 31, 2023
17 checks passed
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
Signed-off-by: yuval weber <yuval199985@gmail.com>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: anton.lysina <alysina@gmail.com>
@yuvalweber yuvalweber deleted the profiling branch March 5, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants