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 support to customize HPA name #3068

Merged
merged 23 commits into from
Jun 20, 2022
Merged

Add support to customize HPA name #3068

merged 23 commits into from
Jun 20, 2022

Conversation

aviadlevy
Copy link
Contributor

@aviadlevy aviadlevy commented May 25, 2022

Provide a description of what has been changed

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 and is aligned with our changelog requirements

Fixes #3057
Docs PR kedacore/keda-docs#768

Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
@aviadlevy aviadlevy marked this pull request as ready for review May 25, 2022 12:56
@aviadlevy aviadlevy requested a review from a team as a code owner May 25, 2022 12:56
Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>

Signed-off-by: Aviad Levy <aviadlevy1@gmail.com>
@aviadlevy aviadlevy changed the title Add support to custom hpa name Add support to customize HPA name May 25, 2022
@zroubalik
Copy link
Member

@aviadlevy you should also run make build to regenerate ScaledObject resources, as this PR adds a new field.

@aviadlevy
Copy link
Contributor Author

@aviadlevy you should also run make build to regenerate ScaledObject resources, as this PR adds a new field.

No git changes after make build

keda on  main [⇡] via 🐹 v1.17.10 ➜  make build     
/Users/aviadl/Code/keda/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags "-X=github.com/kedacore/keda/v2/version.GitCommit=64647753313420a59b0c0c2f764e8347c9770dce -X=github.com/kedacore/keda/v2/version.Version=main" -o bin/keda main.go
GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags "-X=github.com/kedacore/keda/v2/version.GitCommit=64647753313420a59b0c0c2f764e8347c9770dce -X=github.com/kedacore/keda/v2/version.Version=main" -o bin/keda-adapter adapter/main.go

What am I missing? What did you expected to see?

@zroubalik
Copy link
Member

zroubalik commented May 26, 2022

@aviadlevy you should also run make build to regenerate ScaledObject resources, as this PR adds a new field.

No git changes after make build

keda on  main [⇡] via 🐹 v1.17.10 ➜  make build     
/Users/aviadl/Code/keda/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags "-X=github.com/kedacore/keda/v2/version.GitCommit=64647753313420a59b0c0c2f764e8347c9770dce -X=github.com/kedacore/keda/v2/version.Version=main" -o bin/keda main.go
GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags "-X=github.com/kedacore/keda/v2/version.GitCommit=64647753313420a59b0c0c2f764e8347c9770dce -X=github.com/kedacore/keda/v2/version.Version=main" -o bin/keda-adapter adapter/main.go

What am I missing? What did you expected to see?

What about make manifests?

The new field should be present in the CRD yaml file.

controllers/keda/hpa.go Outdated Show resolved Hide resolved
controllers/keda/hpa.go Outdated Show resolved Hide resolved
Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
@aviadlevy
Copy link
Contributor Author

aviadlevy commented May 26, 2022

@aviadlevy you should also run make build to regenerate ScaledObject resources, as this PR adds a new field.

No git changes after make build

keda on  main [⇡] via 🐹 v1.17.10 ➜  make build     
/Users/aviadl/Code/keda/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags "-X=github.com/kedacore/keda/v2/version.GitCommit=64647753313420a59b0c0c2f764e8347c9770dce -X=github.com/kedacore/keda/v2/version.Version=main" -o bin/keda main.go
GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags "-X=github.com/kedacore/keda/v2/version.GitCommit=64647753313420a59b0c0c2f764e8347c9770dce -X=github.com/kedacore/keda/v2/version.Version=main" -o bin/keda-adapter adapter/main.go

What am I missing? What did you expected to see?

What about make manifests?

The new field should be present in the CRD yaml file.

make generate did the trick, though it seems like too many changes: 16b2a01
@zroubalik Do you want me to revert it and leave only the new fields that added?

Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
@zroubalik
Copy link
Member

make generate did the trick, though it seems like too many changes: 16b2a01 @zroubalik Do you want me to revert it and leave only the new fields that added?

Would be nice if you could keep here in the PR only relevant changes. Would you mind opening a new PR with the extra files? We need to update them anyway. Thanks!

Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
@aviadlevy
Copy link
Contributor Author

aviadlevy commented May 30, 2022

make generate did the trick, though it seems like too many changes: 16b2a01 @zroubalik Do you want me to revert it and leave only the new fields that added?

Would be nice if you could keep here in the PR only relevant changes. Would you mind opening a new PR with the extra files? We need to update them anyway. Thanks!

Kept only relevant changes.
Once this MR will be merged I'll open MR for general CRD updates

@zroubalik
Copy link
Member

zroubalik commented May 31, 2022

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

Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
@aviadlevy aviadlevy force-pushed the main branch 2 times, most recently from 1c57a93 to 3ef9f29 Compare June 16, 2022 15:08
@aviadlevy
Copy link
Contributor Author

Added e2e tests 😎
Who can review them? @zroubalik @v-shenoy

@JorTurFer
Copy link
Member

JorTurFer commented Jun 16, 2022

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

@aviadlevy
Copy link
Contributor Author

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

I increased the time waiting for hpa in e2e.

locally the e2e is working (see below), but probably on the github runner it takes more time for things to work?

➜  go test -v ./scalers_go/custom_hpa_name/custom_hpa_name_test.go
=== RUN   TestCustomToDefault
    custom_hpa_name_test.go:136: --- validate hpa is with custom name ---
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:140: --- change hpa name ---
    custom_hpa_name_test.go:144: --- validate new hpa is with default name ---
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:148: --- validate hpa with custom name does not exists ---
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
--- PASS: TestCustomToDefault (14.00s)
=== RUN   TestDefaultToCustom
    custom_hpa_name_test.go:136: --- validate hpa is with default name ---
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:140: --- change hpa name ---
    custom_hpa_name_test.go:144: --- validate new hpa is with custom name ---
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:148: --- validate hpa with default name does not exists ---
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
    custom_hpa_name_test.go:171: Waiting for hpa creation
--- PASS: TestDefaultToCustom (12.36s)
PASS
ok      command-line-arguments  26.894s

Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
@JorTurFer
Copy link
Member

JorTurFer commented Jun 16, 2022

/run-e2e custom_hpa*
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.

Looking good! Just a nit in imports.

tests/scalers_go/custom_hpa_name/custom_hpa_name_test.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

zroubalik commented Jun 17, 2022

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

@aviadlevy
Copy link
Contributor Author

@zroubalik @JorTurFer
Can you please define what's missing for this MR to get merged?

@JorTurFer
Copy link
Member

Can you please define what's missing for this MR to get merged?

I think is ready, at least from my side I don't see any other thing to be done, let's wait till @zroubalik too

@zroubalik
Copy link
Member

There's a conflict brought by another PR :( could you please fix that? Then we can merge this PR. Thanks for the contribution!

# Conflicts:
#	CHANGELOG.md
@JorTurFer
Copy link
Member

JorTurFer commented Jun 20, 2022

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

Signed-off-by: aviadlevy <aviadlevy1@gmail.com>
@JorTurFer
Copy link
Member

JorTurFer commented Jun 20, 2022

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

@JorTurFer JorTurFer merged commit 40cd0a9 into kedacore:main Jun 20, 2022
@JorTurFer
Copy link
Member

thanks for this contribution! ❤️

@aviadlevy
Copy link
Contributor Author

thanks for this contribution! ❤️

Thanks. it was fun and very enriching.
Looking forward to contribute more in the future 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to customize HPA name
5 participants