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

🌱 Add test utils for cluster class #4986

Merged
merged 1 commit into from
Jul 26, 2021
Merged

🌱 Add test utils for cluster class #4986

merged 1 commit into from
Jul 26, 2021

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
This is part of the activities for the implementation of the ClusterClass proposal.

This PR add a first set of test utils/utils that will simplify the work for #4970 and #4971 (and also subsequent PRs)

@killianmuldoon

@k8s-ci-robot k8s-ci-robot requested a review from enxebre July 21, 2021 15:48
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 21, 2021
@srm09
Copy link
Contributor

srm09 commented Jul 21, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2021
@sbueringer
Copy link
Member

@fabriziopandini Thank you, looks great! I'll start using it tomorrow on my PR.

@fabriziopandini fabriziopandini force-pushed the cluster-class-testutil branch from c062d75 to d48717c Compare July 22, 2021 13:12
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2021
@fabriziopandini fabriziopandini force-pushed the cluster-class-testutil branch from d48717c to 385cbad Compare July 22, 2021 13:25
@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2021
This was referenced Jul 22, 2021
Closed
Closed
@stmcginnis
Copy link
Contributor

/lgtm

Though looks like this needs to be rebased now.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2021
@fabriziopandini
Copy link
Member Author

fabriziopandini commented Jul 22, 2021 •
edited
Loading

We are changing a little bit the APIs (@vincepri is opening a PR ATM), and this will make explicit when a controlPlane template has a reference so we can drop getNestedRef or to scope it usages only to tests

@fabriziopandini
Copy link
Member Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021
@sbueringer sbueringer mentioned this pull request Jul 22, 2021
Closed
@fabriziopandini fabriziopandini force-pushed the cluster-class-testutil branch from 385cbad to 902f840 Compare July 22, 2021 18:35
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 22, 2021
@fabriziopandini
Copy link
Member Author

/hold cancel

Given #5000 getNestedRef should not be used anymore for inspection so I enforced checks that the reference should be “complete”

@sbueringer @killianmuldoon ^^

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021
@sbueringer
Copy link
Member

sbueringer commented Jul 22, 2021 •
edited
Loading

@fabriziopandini @killianmuldoon I'm extending the fake objects currently on my PR to cover the whole functionality. (just fyi)

@fabriziopandini fabriziopandini mentioned this pull request Jul 22, 2021
Merged
@fabriziopandini fabriziopandini force-pushed the cluster-class-testutil branch from 902f840 to c99c48a Compare July 22, 2021 19:20
@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2021
@ykakarap ykakarap mentioned this pull request Jul 23, 2021
Merged
@sbueringer sbueringer mentioned this pull request Jul 23, 2021
Merged
vincepri
vincepri approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 Jul 23, 2021
@sbueringer
Copy link
Member

sbueringer commented Jul 23, 2021 •
edited
Loading

@fabriziopandini I think this has to be rebased after the merge of #5000 and fixed. IIrc just a minor fix in fakeClusterClass

@fabriziopandini fabriziopandini force-pushed the cluster-class-testutil branch from c99c48a to fb17129 Compare July 26, 2021 11:06
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2021
@fabriziopandini
Copy link
Member Author

@sbueringer @vincepri rebased on top of #5000 (control plane machine infrastructure is now an explicit field in ClusterClass)

@fabriziopandini
Copy link
Member Author

/retest

@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit 19d190e into kubernetes-sigs:master Jul 26, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone Jul 26, 2021
@fabriziopandini fabriziopandini deleted the cluster-class-testutil branch July 26, 2021 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ykakarap ykakarap ykakarap left review comments

@sbueringer sbueringer sbueringer left review comments

@stmcginnis stmcginnis stmcginnis left review comments

@vincepri vincepri vincepri approved these changes

@enxebre enxebre Awaiting requested review from enxebre

Assignees

@vincepri vincepri

@stmcginnis stmcginnis

@sbueringer sbueringer

@srm09 srm09

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
v0.4
Development

Successfully merging this pull request may close these issues.
7 participants
@fabriziopandini @srm09 @sbueringer @stmcginnis @k8s-ci-robot @vincepri @ykakarap