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

chore: bump deps and get rid of github.com/Azure/azure-service-bus-go #3639

Merged
merged 17 commits into from
Sep 15, 2022
Merged

chore: bump deps and get rid of github.com/Azure/azure-service-bus-go #3639

merged 17 commits into from
Sep 15, 2022

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Sep 1, 2022 •
edited
Loading

Signed-off-by: Jorge Turrado jorge_turrado@hotmail.es

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #3394
Fixes #2986
Related to kedacore/keda-docs#930

@JorTurFer JorTurFer requested a review from a team as a code owner September 1, 2022 15:58
@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 2, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer enabled auto-merge (squash) September 2, 2022 14:39
@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 9, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

please @v-shenoy , take a look to servicebus part. I had to reimplement part of it during the dep migration.
I have seen also that we could use NewDefaultAzureCredential for other services if we use the SDK instead of http request. Do you think that migrating from http request to the azure SDK is worth?
using NewDefaultAzureCredential we can use az-cli credentials to validate that pod identity workflow works, in fact, it's the way that we follow in dotnet to unify the access using managed credentials. Could I be missing something?

@JorTurFer
Copy link
Member Author

Even snyk reports a new vulnerability introduced, I think it's a false detection because we already use the fixed version in this PR:
https://github.com/kedacore/keda/pull/3639/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6L283

image

It's seems like the issue about restful dep we talked about some week ago

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 9, 2022 •
edited by github-actions bot
Loading

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

@v-shenoy
Copy link
Contributor

v-shenoy commented Sep 10, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

I'm checking the e2e tests, I'd say that the error is in the e2e test itself. 👀

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 10, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 10, 2022 •
edited by github-actions bot
Loading

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

@v-shenoy
Copy link
Contributor

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

On a related note, could you move the utility functions used in the service bus tests (setting/ cleaning up queue / topic / subscription, etc) into a separate package so that we can reuse them?

If not here, I'll try to do them as a part of #3607

@JorTurFer JorTurFer changed the title chore: bump deps chore: bump deps and get rid of github.com/Azure/azure-service-bus-go Sep 10, 2022
@JorTurFer
Copy link
Member Author

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

On a related note, could you move the utility functions used in the service bus tests (setting/ cleaning up queue / topic / subscription, etc) into a separate package so that we can reuse them?

If not here, I'll try to do them as a part of #3607

I'll try it

v-shenoy
v-shenoy reviewed Sep 10, 2022
View reviewed changes
pkg/scalers/azure_servicebus_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/azure_servicebus_scaler.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 10, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 10, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer mentioned this pull request Sep 11, 2022
Closed
1 task
@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 11, 2022 •
edited by github-actions bot
Loading

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

v-shenoy
v-shenoy approved these changes Sep 11, 2022
View reviewed changes
Copy link
Contributor

@v-shenoy v-shenoy left a comment

Choose a reason for hiding this comment

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

LGTM. The only scenario that concerns me is pod identity, wish there was a way we could check it easily right now, considering we lack e2e tests.

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 11, 2022 •
edited
Loading

LGTM. The only scenario that concerns me is pod identity, wish there was a way we could check it easily right now, considering we lack e2e tests.

I'll spawn aad-pod-identity on my cluster and try it, but it's true, having aad-pod-identity e2e test would be nice.
I can add an e2e test for it, but needed resources are still pending I guess #2895

@v-shenoy
Copy link
Contributor

v-shenoy commented Sep 11, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 11, 2022 •
edited
Loading

I have tried in my cluster with aad-pod-identity and it works. I have followed this doc to configure aad-pod-identity using the addon, and it has been really easy.

Covering the case with a test is also easy because we only need to copy-paste the workload-identity e2e test and change the trigger authentication from azure-workload to azure once we have the infra ready and patch KEDA deployments, as we are already doing with the service account.

@JorTurFer JorTurFer mentioned this pull request Sep 12, 2022
Closed
@v-shenoy
Copy link
Contributor

v-shenoy commented Sep 13, 2022 •
edited
Loading

Once this is merged, I can tackle the feature request for SAS strings as we'll have the new SDK in use.

v-shenoy
v-shenoy suggested changes Sep 13, 2022
View reviewed changes
pkg/scalers/azure_servicebus_scaler.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 13, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 13, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer
chore: bump deps
cc41d5e 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix pkg issue
c4ccc47 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
use github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus (1)
427c215 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
use github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus (2)
6f4a0e4 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix style
604d189 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
bump some deps
abc2a77 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
bump some deps
819ba2f 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix some styles
879fb74 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
undo changes in devcontainer dockerfile
1590afd 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
update changelog
5a19217 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix errors in test and panic
4e801d3 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
.
62ee7ad 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
fix style
85a78ed 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
split pod identity cases
cc7c502 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
use namespace instead of fullyQualifiedNamespace
c36a688 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
remove unecessary test
c5835cc 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
unify token providers and allow setting the clientID
ad919c6 
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 14, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer mentioned this pull request Sep 14, 2022
Merged
4 tasks
@JorTurFer JorTurFer merged commit 77f31b7 into kedacore:main Sep 15, 2022
@JorTurFer JorTurFer deleted the bump-deps branch September 15, 2022 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v-shenoy v-shenoy v-shenoy approved these changes

@zroubalik zroubalik zroubalik approved these changes

@tomkerkhove tomkerkhove Awaiting requested review from tomkerkhove

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

Successfully merging this pull request may close these issues.

Migrate from azure-service-bus-go to azservicebus Update github.com/Azure/azure-event-hubs-go/v3
4 participants
@JorTurFer @v-shenoy @zroubalik @tomkerkhove