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 HPA Custom Behavior #4391

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Add HPA Custom Behavior #4391

merged 7 commits into from
Oct 10, 2023

Conversation

saedx1
Copy link
Contributor

@saedx1 saedx1 commented Sep 20, 2023

Proposed changes

There is no issue for this pull request. It is a very basic PR.
It adds the ability to define spec.behavior for an HorizontalPodAutoscaler.

  • Defines it in the values.yaml file; defaults to empty.
  • References it in the controller hpa template.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@saedx1 saedx1 requested a review from a team as a code owner September 20, 2023 11:40
@github-actions github-actions bot added the helm_chart Pull requests that update the Helm Chart label Sep 20, 2023
@vepatel
Copy link
Contributor

vepatel commented Sep 20, 2023

Hi @saedx1 thanks for opening a PR, please make sure to follow https://github.com/nginxinc/kubernetes-ingress/blob/main/CONTRIBUTING.md#open-a-pull-request

@saedx1
Copy link
Contributor Author

saedx1 commented Sep 20, 2023

Hi @saedx1 thanks for opening a PR, please make sure to follow https://github.com/nginxinc/kubernetes-ingress/blob/main/CONTRIBUTING.md#open-a-pull-request

Hi @vepatel , Let me know if an issue is still needed in this case. The proposed change is trivial and does not affect current functionality, however, if you think an issue is needed for further discussion, I will open one.

@vepatel
Copy link
Contributor

vepatel commented Sep 20, 2023

@saedx1 Yes would really appreciate, as it helps us prioritise reviews and tests against community PRs :)
Also please make sure to:

Fork the repo, create a branch, submit a PR when your changes are tested and ready for review

@saedx1
Copy link
Contributor Author

saedx1 commented Sep 20, 2023

@saedx1 Yes would really appreciate, as it helps us prioritise reviews and tests against community PRs :) Also please make sure to:

Fork the repo, create a branch, submit a PR when your changes are tested and ready for review

Done

@brianehlert brianehlert linked an issue Sep 20, 2023 that may be closed by this pull request
@haywoodsh haywoodsh self-assigned this Oct 3, 2023
@haywoodsh
Copy link
Contributor

Thanks @saedx1! I deployed the helm chart and the HPA resource was configured correctly.
Could you please also update the readme and the docs to include the helm parameter you added?
https://github.com/nginxinc/kubernetes-ingress/blob/v3.3.0/deployments/helm-chart/README.md
https://github.com/nginxinc/kubernetes-ingress/blob/v3.3.0/docs/content/installation/installation-with-helm.md

@lucacome
Copy link
Member

lucacome commented Oct 3, 2023

The right file to update is https://github.com/nginxinc/kubernetes-ingress/blob/main/charts/nginx-ingress/README.md. It's copied automatically to the docs on release.

@github-actions github-actions bot added documentation Pull requests/issues for documentation and removed helm_chart Pull requests that update the Helm Chart labels Oct 3, 2023
- Defines a `controller.autoscaling.behavior` object and defaults it to an empty object `{}`.
- References the object in the `templates/controller-hpa.yaml`.
@github-actions github-actions bot added the helm_chart Pull requests that update the Helm Chart label Oct 3, 2023
@saedx1
Copy link
Contributor Author

saedx1 commented Oct 3, 2023

@haywoodsh @lucacome Done :D

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #4391 (1dcffca) into main (95bcf9f) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4391      +/-   ##
==========================================
+ Coverage   51.95%   51.96%   +0.01%     
==========================================
  Files          59       59              
  Lines       16956    16956              
==========================================
+ Hits         8809     8811       +2     
+ Misses       7850     7848       -2     
  Partials      297      297              

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@saedx1
Copy link
Contributor Author

saedx1 commented Oct 5, 2023

@haywoodsh @lucacome Can we merge this?

@coolbry95
Copy link
Contributor

This should have a doc change.

@saedx1
Copy link
Contributor Author

saedx1 commented Oct 10, 2023

This should have a doc change.

It does. See comments above also

@coolbry95
Copy link
Contributor

This should have a doc change.

It does. See comments above also

That will only add it to the readme.

This is when I originally implemented the HPA which adds docs so that they will show up in the website.
#3276

@jjngx
Copy link
Contributor

jjngx commented Oct 10, 2023

@saedx1 could you please update your branch to get it read to be merged?

@lucacome lucacome merged commit e390768 into nginxinc:main Oct 10, 2023
62 checks passed
@jasonwilliams14
Copy link
Contributor

@saedx1 Thanks for the contribution and great work! We appreciate it very much!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Allow an HPA behavior object to be passed
7 participants