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 openapi spec #1512

Merged
merged 12 commits into from
Jan 20, 2021
Merged

Add openapi spec #1512

merged 12 commits into from
Jan 20, 2021

Conversation

surki
Copy link
Contributor

@surki surki commented Jan 19, 2021

Signed-off-by: Suresh Kumar Ponnusamy suresh.ponnusamy@freshworks.com

Add OpenAPI spec support, without which k8s apiserver log is flooded with errors/retries.
This follows similar upstream changes done here

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)
  • Changelog has been updated

Fixes #1503

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
Suresh Kumar Ponnusamy added 2 commits January 19, 2021 16:56
Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
@surki
Copy link
Contributor Author

surki commented Jan 19, 2021

@zroubalik looks like we move pkg/generated this away, generate again and compare. Should I move pkg/generated/openapi into adapter/generated or somewhere else?

Or should I move the openapi spec generation logic into hack/update-codegen.sh? (not sure the role of make generate)

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
@surki surki force-pushed the fix-openapi-spec-errors branch from 6804b6d to b5d4408 Compare January 19, 2021 11:57
@zroubalik
Copy link
Member

Thanks for doing this!

@zroubalik looks like we move pkg/generated this away, generate again and compare. Should I move pkg/generated/openapi into adapter/generated or somewhere else?

Or should I move the openapi spec generation logic into hack/update-codegen.sh? (not sure the role of make generate)

I'd move it into adapter/generated.

adapter/main.go Outdated Show resolved Hide resolved
adapter/main.go Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Suresh Kumar Ponnusamy and others added 5 commits January 20, 2021 10:35
Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
Co-authored-by: Zbynek Roubalik <726523+zroubalik@users.noreply.github.com>
Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
@surki surki force-pushed the fix-openapi-spec-errors branch from e90a5ff to 2ac3417 Compare January 20, 2021 08:22
@zroubalik
Copy link
Member

@surki you can ignore the failing static check (I will address that in a separate PR), just please add exclude-rule for the staticcheck linter for the pkg/scalers/stackdriver_client.go in https://github.com/kedacore/keda/blob/main/.golangci.yml#L50 and please add there a comment with explanation and link to this PR.

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
@surki
Copy link
Contributor Author

surki commented Jan 20, 2021

@zroubalik looks like the CI / Validate PR (pull_request) failure is due to this . We appear to be using tools.go from main branch, so it is not pulling new dependency that is introduced in this PR.

@zroubalik
Copy link
Member

@zroubalik looks like the CI / Validate PR (pull_request) failure is due to this . We appear to be using tools.go from main branch, so it is not pulling new dependency that is introduced in this PR.

@surki Sorry I am not sure I fully understand, that line in pr-validation.yml is about the container that's being used to do the build, you can see the Dockerfile for this container here: https://github.com/kedacore/keda/blob/main/tools/build-tools.Dockerfile

@surki
Copy link
Contributor Author

surki commented Jan 20, 2021

@zroubalik looks like the CI / Validate PR (pull_request) failure is due to this . We appear to be using tools.go from main branch, so it is not pulling new dependency that is introduced in this PR.

@surki Sorry I am not sure I fully understand, that line in pr-validation.yml is about the container that's being used to do the build, you can see the Dockerfile for this container here: https://github.com/kedacore/keda/blob/main/tools/build-tools.Dockerfile

The new dependency we have added here in tools.go is not being pulled in (but looks like controller-gen etc are already pulled in as part of Docker image).

I could think of two options:

  • Add a step in workflow file to sync go modules. Something like
@@ -23,6 +23,9 @@ jobs:
           restore-keys: |
             ${{ runner.os }}-go-
 
+      - name: Go modules sync
+        run: go mod tidy
+
       - name: Verify Generated clientset is up to date
         run: make clientset-verify
  • Update the build-tools.Dockerfile to install openapi-gen

I would suggest to go with first option. are you okay if I do that?

@zroubalik
Copy link
Member

@surki gotcha. Syncing the go.mod sounds like a good idea!

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
Makefile Show resolved Hide resolved
It appears go1.14 introduced different behavior when running 'go list' with
vendor directory (which our Makefile seems to create temporarily and not
cleanup). This change makes 'go list' work with even when vendor directory
is present

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>
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 zroubalik merged commit 683da3b into kedacore:main Jan 20, 2021
@surki surki deleted the fix-openapi-spec-errors branch January 20, 2021 12:28
ycabrer pushed a commit to ycabrer/keda that referenced this pull request Mar 1, 2021
* Add openapi spec

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Update changelog

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Update CHANGELOG.md

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Move generated openapi file

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Update generated code

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Fix import grouping

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Update autogenerated tag

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Update CHANGELOG.md

Co-authored-by: Zbynek Roubalik <726523+zroubalik@users.noreply.github.com>
Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Disable static linter for a stackdriver scaler

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Sync go module in workflow run

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

* Fix 'go list' invocation.

It appears go1.14 introduced different behavior when running 'go list' with
vendor directory (which our Makefile seems to create temporarily and not
cleanup). This change makes 'go list' work with even when vendor directory
is present

Signed-off-by: Suresh Kumar Ponnusamy <suresh.ponnusamy@freshworks.com>

Co-authored-by: Zbynek Roubalik <726523+zroubalik@users.noreply.github.com>
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.

Rate limiting of v1beta1.external.metrics.k8s.io
2 participants