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 GCP stackdriver aggregation parameters #3070

Merged
merged 15 commits into from
Jun 29, 2022

Conversation

RamCohen
Copy link
Contributor

@RamCohen RamCohen commented May 25, 2022

Fixes #3008

Docs: kedacore/keda-docs#776

Signed-off-by: Ram Cohen ram.cohen@gmail.com

@RamCohen RamCohen requested a review from a team as a code owner May 25, 2022 14:59
@RamCohen
Copy link
Contributor Author

Will open a documentation PR if this is tentatively approved

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 in general, nice impovement, thanks!

pkg/scalers/gcp_pubsub_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/gcp_stackdriver_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/gcp_pubsub_scaler.go Outdated Show resolved Hide resolved
@RamCohen
Copy link
Contributor Author

RamCohen commented Jun 8, 2022

Not sure what to do with the annoying unrelated static check failure:
pkg/scalers/azure_blob_scaler.go:131:102: string true has 3 occurrences, make it a constant (goconst)

@JorTurFer
Copy link
Member

Not sure what to do with the annoying unrelated static check failure:
pkg/scalers/azure_blob_scaler.go:131:102: string true has 3 occurrences, make it a constant (goconst)

IDK why this happens only in your PR and not in others... You could just refactor it a bit, something like this:

	if val, ok := config.TriggerMetadata["useAAdPodIdentity"]; ok {
		value, err := strconv.ParseBool(val)
		if err != nil {
			return nil, "", err
		}
		if config.PodIdentity == "" && value {
			config.PodIdentity = kedav1alpha1.PodIdentityProviderAzure
		}
	}

@zroubalik
Copy link
Member

Not sure what to do with the annoying unrelated static check failure:
pkg/scalers/azure_blob_scaler.go:131:102: string true has 3 occurrences, make it a constant (goconst)

IDK why this happens only in your PR and not in others... You could just refactor it a bit, something like this:

	if val, ok := config.TriggerMetadata["useAAdPodIdentity"]; ok {
		value, err := strconv.ParseBool(val)
		if err != nil {
			return nil, "", err
		}
		if config.PodIdentity == "" && value {
			config.PodIdentity = kedav1alpha1.PodIdentityProviderAzure
		}
	}

It is happening because another "true" has been added in this PR in pkg/scalers/stackdriver_client.go.

@JorTurFer
Copy link
Member

It is happening because another "true" has been added in this PR in pkg/scalers/stackdriver_client.go.

lol, good catch, I didn't see it 👁️

@RamCohen
Copy link
Contributor Author

RamCohen commented Jun 9, 2022

changed the switch statement to use "count_true" and "count_false" so that should solve it

@zroubalik
Copy link
Member

zroubalik commented Jun 13, 2022

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

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

@zroubalik
Copy link
Member

zroubalik commented Jun 14, 2022

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

// Zbynek update: transient env failure.

@zroubalik
Copy link
Member

zroubalik commented Jun 14, 2022

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

@zroubalik
Copy link
Member

@RamCohen e2e tests failed on this one. Btw we also have started a migration of e2e tests from TS to Go, do you think you can try to tackle this? Thanks!

@RamCohen
Copy link
Contributor Author

RamCohen commented Jun 17, 2022 via email

@JorTurFer
Copy link
Member

I saw that but I can't see how to get to the logs to see what happens

Hey,
You can click in the link that the GH action has updated and scroll inside the step "Run end to end tests". This is the link to one of the logs
image

@zroubalik
Copy link
Member

zroubalik commented Jun 20, 2022

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

@zroubalik
Copy link
Member

@RamCohen #2737

Here you can see initial migration to Golang based tests. Also the readme has been updated: https://github.com/kedacore/keda/blob/main/tests/README.md

@tomkerkhove
Copy link
Member

tomkerkhove commented Jun 21, 2022

/run-e2e gcp*

Update: You can check the progress here

@RamCohen
Copy link
Contributor Author

looks like no tests were actually run

@zroubalik
Copy link
Member

@JorTurFer mind looking at this^

@zroubalik
Copy link
Member

zroubalik commented Jun 23, 2022

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

@zroubalik
Copy link
Member

zroubalik commented Jun 24, 2022

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

@zroubalik
Copy link
Member

zroubalik commented Jun 27, 2022

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

@zroubalik
Copy link
Member

zroubalik commented Jun 27, 2022

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

@zroubalik
Copy link
Member

@RamCohen could you please update Changelog?

Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented Jun 29, 2022

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

@zroubalik zroubalik merged commit b810307 into kedacore:main Jun 29, 2022
@ritmas
Copy link

ritmas commented Jul 11, 2022

When could this be released @zroubalik @RamCohen ?

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.

Introduce metric aggregation/alignment in GCP Stackdriver scaler
5 participants