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 info in kubectl get triggerauthentication #4668

Merged
merged 26 commits into from
Jul 3, 2023

Conversation

SpiritZhou
Copy link
Contributor

@SpiritZhou SpiritZhou commented Jun 9, 2023

Add scaledobjetc/socaledjob info when kubectl get triggerauthentication/clustertriggerauthentication.
Here is more info: #796

Example:
image

Checklist

  • [N/A] When introducing a new scaler, I agree with the scaling governance policy
  • I have verified that my change is according to the deprecations & breaking changes policy
  • Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements
  • [N/A] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • [N/A] A PR is opened to update the documentation on (repo) (if applicable)
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #796

@SpiritZhou SpiritZhou requested a review from a team as a code owner June 9, 2023 09:18
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.

As discussed, would you mind adding some tests + example output please?

CHANGELOG.md Outdated Show resolved Hide resolved
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, we should def add a tests for this.

apis/keda/v1alpha1/triggerauthentication_types.go Outdated Show resolved Hide resolved
apis/keda/v1alpha1/triggerauthentication_types.go Outdated Show resolved Hide resolved
controllers/keda/util/string_lists.go Outdated Show resolved Hide resolved
@SpiritZhou
Copy link
Contributor Author

It seems like adding infrastructure test should provide a new PR? I will add a PR about the test after PR merging. @tomkerkhove @zroubalik

@SpiritZhou SpiritZhou force-pushed the spiritzhou/addInfoInTA branch 5 times, most recently from 290a801 to 414a877 Compare June 13, 2023 13:33
SpiritZhou and others added 15 commits June 14, 2023 10:08
By making a contribution to this project, I certify that:

    (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

    (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

    (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

    (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: spiritzhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@tomkerkhove
Copy link
Member

It depends what you need. Our cluster is managed in another PR but you should be good for this PR.

For e2e tests we deploy resources to the cluster which is part of the e2e rest itself so we can do it in this PR.

Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@SpiritZhou
Copy link
Contributor Author

Thanks, @tomkerkhove. I added a e2e test in the PR.

controllers/keda/util/string_lists.go Show resolved Hide resolved
controllers/keda/util/string_lists.go Show resolved Hide resolved
controllers/keda/triggerauthentication_controller.go Outdated Show resolved Hide resolved
controllers/keda/triggerauthentication_controller.go Outdated Show resolved Hide resolved
controllers/keda/triggerauthentication_controller.go Outdated Show resolved Hide resolved
pkg/util/status.go Outdated Show resolved Hide resolved
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.

Would be nice to add a functional test here:

Describe("functional tests", func() {

(create SO _ check TA status, remove SO check TA status again, create 2 SOs check TA).

And also create a separate test for scaledjob_controller that is testing the same.

Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@zroubalik zroubalik closed this Jun 21, 2023
@zroubalik zroubalik reopened this Jun 21, 2023
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, just a few nits to be done. In general it is better to add comments on every functions that do something non-trivial.

pkg/status/status.go Outdated Show resolved Hide resolved
pkg/status/status.go Outdated Show resolved Hide resolved
tests/helper/helper.go Show resolved Hide resolved
controllers/keda/triggerauthentication_controller.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

zroubalik commented Jun 21, 2023

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

Signed-off-by: Tom Kerkhove <[email protected]>
@tomkerkhove
Copy link
Member

tomkerkhove commented Jun 21, 2023

/run-e2e internal

Update: You can check the progress here

@SpiritZhou SpiritZhou force-pushed the spiritzhou/addInfoInTA branch 2 times, most recently from 6065ef8 to e949528 Compare June 30, 2023 04:00
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@tomkerkhove
Copy link
Member

tomkerkhove commented Jun 30, 2023

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

@tomkerkhove
Copy link
Member

LGTM, but I'll leave it up to @zroubalik

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.

LGTM

Thanks!

@zroubalik zroubalik merged commit 9632275 into kedacore:main Jul 3, 2023
20 checks passed
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.

kubectl get triggerauthentication should show information about scaledobjects
3 participants