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.

Sign up for GitHub

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

Jump to bottom

🌱 Remove infraServer and credsSecretName from supervisor ClusterClass #2862

Merged
merged 1 commit into from
Apr 3, 2024
Merged

🌱 Remove infraServer and credsSecretName from supervisor ClusterClass #2862

merged 1 commit into from
Apr 3, 2024

Conversation

zhanggbj
Copy link
Contributor

@zhanggbj zhanggbj commented Mar 28, 2024 •
edited
Loading

What this PR does / why we need it:
Refine make generate-flavors to not generate unused variables for supervisor mode.

  • infraServer
  • credsSecretName

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2829

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 28, 2024
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 28, 2024
@zhanggbj zhanggbj changed the title Remove infraServer and credsSecretName from supervisor ClusterClass 🌱 Remove infraServer and credsSecretName from supervisor ClusterClass Mar 28, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 28, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.81%. Comparing base (80d2f2a) to head (7b5f0ed).
Report is 13 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2862      +/-   ##
==========================================
- Coverage   63.97%   63.81%   -0.17%     
==========================================
  Files         160      160              
  Lines        9375     9375              
==========================================
- Hits         5998     5983      -15     
- Misses       2917     2928      +11     
- Partials      460      464       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Mar 29, 2024
@zhanggbj
Refine flavor-gen for supervisor mode templates
7b5f0ed 
Remove unused InfraServer and credsSecretName from supervisor
ClusterClass amd topololy templates.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2024
fabriziopandini
fabriziopandini reviewed Mar 29, 2024
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

only one small nit, otherwise lgtm

packaging/flavorgen/flavors/clusterclass_generators.go Show resolved Hide resolved
@zhanggbj
Copy link
Contributor Author

zhanggbj commented Apr 2, 2024

All comments are addressed, and this is ready for another review.

@chrischdi
Copy link
Member

/test help

@k8s-ci-robot
Copy link
Contributor

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-vsphere-e2e-blocking-main
  • /test pull-cluster-api-provider-vsphere-e2e-conformance-ci-latest-main
  • /test pull-cluster-api-provider-vsphere-e2e-conformance-main
  • /test pull-cluster-api-provider-vsphere-e2e-main
  • /test pull-cluster-api-provider-vsphere-e2e-supervisor-main
  • /test pull-cluster-api-provider-vsphere-e2e-upgrade-1-29-1-30-main
  • /test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
  • /test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main
  • /test pull-cluster-api-provider-vsphere-test-integration-main
  • /test pull-cluster-api-provider-vsphere-test-main
  • /test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-provider-vsphere-apidiff-main
  • pull-cluster-api-provider-vsphere-e2e-blocking-main
  • pull-cluster-api-provider-vsphere-test-integration-main
  • pull-cluster-api-provider-vsphere-test-main
  • pull-cluster-api-provider-vsphere-verify-main

In response to this:

/test help

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.

@chrischdi
Copy link
Member

/test pull-cluster-api-provider-vsphere-e2e-main
/test pull-cluster-api-provider-vsphere-e2e-supervisor-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

@zhanggbj
Copy link
Contributor Author

zhanggbj commented Apr 2, 2024

/test pull-cluster-api-provider-vsphere-e2e-main

@chrischdi
Copy link
Member

/retest

To see if this is a new error or flake

@chrischdi
Copy link
Member

Note: already failed two times in a row, so we might have an issue here.

@chrischdi
Copy link
Member

Either this PR introduces flakyness or we also have it on main.

testgrid for e2e-main shows green currently, but this runs only daily: https://testgrid.k8s.io/sig-cluster-lifecycle-cluster-api-provider-vsphere#periodic-e2e-main

@sbueringer
Copy link
Member

@chrischdi we can trigger the periodic manually a few times (I did it now)

@sbueringer
Copy link
Member

/retest

@sbueringer
Copy link
Member

/test pull-cluster-api-provider-vsphere-e2e-main

@sbueringer
Copy link
Member

I think this PR is safe to merge.

Effectively we only changed the supervisor cluster template & ClusterClass. None of the test cases that previously failed are supervisor tests.

/approve

@chrischdi WDYT?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2024
@chrischdi
Copy link
Member

agree

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 752200701e361aab21a80f7574342dca6156a972

@k8s-ci-robot k8s-ci-robot merged commit f4a53e8 into kubernetes-sigs:main Apr 3, 2024
26 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.10 milestone Apr 3, 2024
@zhanggbj zhanggbj mentioned this pull request Oct 31, 2024
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laozc laozc Awaiting requested review from laozc

@sbueringer sbueringer Awaiting requested review from sbueringer

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

Assignees

@chrischdi chrischdi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.

Remove unused variables from supervisor ClusterClass and Cluster template
5 participants
@zhanggbj @chrischdi @k8s-ci-robot @sbueringer @fabriziopandini