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

fix: controller.topologySpreadConstraints schema #3527

Merged
merged 5 commits into from
Feb 9, 2023

Conversation

marcuz
Copy link
Contributor

@marcuz marcuz commented Feb 7, 2023

Proposed changes

Fixes error:

controller.topologySpreadConstraints: Invalid type. Expected: object, given: array

Expected YAML:

controller:
  topologySpreadConstraints:
    - maxSkew: 1
      topologyKey: topology.kubernetes.io/zone
      whenUnsatisfiable: DoNotSchedule
      labelSelector:
        matchLabels:
          app: nginx-ingress
    - maxSkew: 1
      topologyKey: kubernetes.io/hostname
      whenUnsatisfiable: DoNotSchedule
      labelSelector:
        matchLabels:
          app: nginx-ingress

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

@marcuz marcuz requested a review from a team as a code owner February 7, 2023 11:49
@github-actions github-actions bot added documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart labels Feb 7, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2023

Codecov Report

Merging #3527 (49a6be2) into main (9b435ad) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 49a6be2 differs from pull request most recent head 4fb481f. Consider uploading reports for the commit 4fb481f to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3527      +/-   ##
==========================================
+ Coverage   51.98%   52.00%   +0.02%     
==========================================
  Files          60       60              
  Lines       16816    16816              
==========================================
+ Hits         8742     8746       +4     
+ Misses       7777     7775       -2     
+ Partials      297      295       -2     
Impacted Files Coverage Δ
internal/k8s/configuration.go 95.79% <0.00%> (+0.36%) ⬆️

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

Copy link
Member

@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

Hi @marcuz I think we just need to change the ref to the right definition and everything else can remain the same

deployments/helm-chart/values.schema.json Outdated Show resolved Hide resolved
@lucacome lucacome self-assigned this Feb 7, 2023
deployments/helm-chart/README.md Outdated Show resolved Hide resolved
deployments/helm-chart/values.schema.json Outdated Show resolved Hide resolved
deployments/helm-chart/values.schema.json Outdated Show resolved Hide resolved
deployments/helm-chart/values.yaml Outdated Show resolved Hide resolved
@marcuz
Copy link
Contributor Author

marcuz commented Feb 8, 2023

@lucacome sorry I totally misunderstood the behaviour of $ref here. Pushing a cleanup commit soon!

@vepatel vepatel self-requested a review February 8, 2023 09:57
@github-actions github-actions bot removed the documentation Pull requests/issues for documentation label Feb 8, 2023
@marcuz
Copy link
Contributor Author

marcuz commented Feb 8, 2023

Just changing the $ref as suggested, lint passes successfully:

$ helm lint . -f values.yaml -f values-test.yaml 
==> Linting .

1 chart(s) linted, 0 chart(s) failed

values-test.yaml contains the controller.topologySpreadConstraints as specified above.

marcuz and others added 5 commits February 8, 2023 22:05
Co-authored-by: Luca Comellini <luca.com@gmail.com>
Signed-off-by: Marco Londero <marco.londero@linux.it>
Co-authored-by: Luca Comellini <luca.com@gmail.com>
Signed-off-by: Marco Londero <marco.londero@linux.it>
Copy link
Member

@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

Thanks @marcuz !

@lucacome lucacome merged commit 27973bf into nginxinc:main Feb 9, 2023
@marcuz
Copy link
Contributor Author

marcuz commented Feb 9, 2023

Thanks @marcuz !

Sorry for all the back and forth. 😄

@marcuz marcuz deleted the fix-tsc branch February 9, 2023 08:16
@cpuengr949
Copy link

cpuengr949 commented Feb 10, 2023

@lucacome I am getting this same error using terraform to build
topologySpreadConstraints = [
{
labelSelector = {
matchLabels = {
app = "nginx-plus-nginx-ingress"
}
}
maxSkew = 1
topologyKey = "kubernetes.io/hostname"
whenUnsatisfiable = "DoNotSchedule"
}
]

Which is creating the code
"topologySpreadConstraints":
- "labelSelector":
"matchLabels":
"app": "nginx-plus-nginx-ingress"
"maxSkew": 1
"topologyKey": "kubernetes.io/hostname"
"whenUnsatisfiable": "DoNotSchedule"

@lucacome
Copy link
Member

@cpuengr949 even with the fix in this PR?

@cpuengr949
Copy link

@lucacome How do I get the fix?
I believe terraform downloads it each time right?
Thank You

@lucacome
Copy link
Member

@cpuengr949 if it's pulling the main branch from here or using the edge helm chart, it should have the fix in this PR, but I don't know what your setup is 🙂

lucacome pushed a commit that referenced this pull request Feb 13, 2023
Signed-off-by: Marco Londero <marco.londero@linux.it>
(cherry picked from commit 27973bf)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm_chart Pull requests that update the Helm Chart
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants