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

feat: avoid-pdb-creation-when-default-backend-disabled-and-replicas-gt-1 #7420

Closed
wants to merge 12 commits into from
Closed

feat: avoid-pdb-creation-when-default-backend-disabled-and-replicas-gt-1 #7420

wants to merge 12 commits into from

Conversation

marcportabellaclotet-mt
Copy link
Contributor

What this PR does / why we need it:

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Which issue/s this PR fixes

Pod disruption budgets should not be created when default-backend deployment creation is set to disabled and default backend replicas are set to more than 1.

How Has This Been Tested?

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Aug 3, 2021
@k8s-ci-robot
Copy link
Contributor

@marcportabellaclotet-mt: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Aug 3, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @marcportabellaclotet-mt. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/helm Issues or PRs related to helm charts labels Aug 3, 2021
@marcportabellaclotet-mt
Copy link
Contributor Author

marcportabellaclotet-mt commented Aug 3, 2021

/assign @rikatz

@marcportabellaclotet-mt
Copy link
Contributor Author

Previous PR was closed due to some conflicts.

As commented by @rikatz , I am marking him to do a fast review

@rikatz
Copy link
Contributor

rikatz commented Aug 5, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 5, 2021
… jobs (#7434)

* helm: add feature to configure request and limit for container in createSecret and patchWebhook job

Signed-off-by: Bhumij Gupta <[email protected]>

* Remove empty line in helm template

Signed-off-by: Bhumij Gupta <[email protected]>

* Add test for admission webhook job container resources

Signed-off-by: Bhumij Gupta <[email protected]>

* Add new line character at the end of charts ci file

Signed-off-by: Bhumij Gupta <[email protected]>
Copy link
Contributor

@rikatz rikatz left a comment

Choose a reason for hiding this comment

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

@marcportabellaclotet-mt thanks for your PR

Just a minor thing here so we can follow k8s api deprecations, and we are good to go.

@@ -1,3 +1,4 @@
{{- if .Values.defaultBackend.enabled -}}
{{- if or (gt (.Values.defaultBackend.replicaCount | int) 1) (gt (.Values.defaultBackend.autoscaling.minReplicas | int) 1) }}
apiVersion: policy/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that I did the wrong thing, rebasing from main branch.
Sorry for the inconvenience.
It this is a problem, I will close the PR.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 5, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marcportabellaclotet-mt
To complete the pull request process, please ask for approval from rikatz after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rikatz
Copy link
Contributor

rikatz commented Aug 6, 2021

yeah, there is some problem with the rebase
steps to rebase:

  • git fetch upstream/main
  • git rebase upstream/main
  • git push -f origin yourbranchname

@marcportabellaclotet-mt
Copy link
Contributor Author

Sorry for the delay.
I rebased my branch, but this PR is not getting updated. Is there any further action I have to take? Create a new PR?
Again, sorry for the confusion, but I never worked with forks before, so I am a bit lost.

@rikatz
Copy link
Contributor

rikatz commented Aug 21, 2021

@marcportabellaclotet-mt no need to ask sorry :)

So we are going to merge dev-v1 in the next days, and it's going to create huge rebases/conflicts, so I would wait before opening a new PR or working in this one :)

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 19, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 19, 2021
@rikatz
Copy link
Contributor

rikatz commented Dec 23, 2021

/lifecycle frozen
Cycling back to this

@k8s-ci-robot
Copy link
Contributor

@rikatz: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/lifecycle frozen
Cycling back to this

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@rikatz
Copy link
Contributor

rikatz commented Dec 29, 2021

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 29, 2021
@@ -2,7 +2,7 @@ apiVersion: v2
name: ingress-nginx
# When the version is modified, make sure the artifacthub.io/changes list is updated
# Also update CHANGELOG.md
version: 3.34.0
version: 3.35.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't bump on this right now

@rikatz
Copy link
Contributor

rikatz commented Dec 29, 2021

@marcportabellaclotet-mt there's something...strange with this PR :)

You are changing some e2e tests, + the deployment manifests.

Right now, can we stick only with the helm chart change?

Thanks!!

@marcportabellaclotet-mt
Copy link
Contributor Author

This PR was opened a long time ago, and the only change I applied at the beginning was the one related to the helm chart.
I am not sure on how to proceed?

  • Fetch upstream : main?
  • Close PR and create a new one?
    Thanks

@rikatz
Copy link
Contributor

rikatz commented Jan 16, 2022

@marcportabellaclotet-mt

git fetch upstream main
git rebase upstream/main
git push origin your-branch

Should do the required.

Otherwise feel free to open a new PR and ping me here and in Slack (slack is better, I keep missing notifications here) and let's get this merged! :)

@marcportabellaclotet-mt
Copy link
Contributor Author

Closed PR, added a new one with the required changes.

#8155

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Issues or PRs related to helm charts cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants