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 script to install Kind for testing with CAPD #5232

Merged
merged 1 commit into from
Sep 23, 2021
Merged

🌱 Add script to install Kind for testing with CAPD #5232

merged 1 commit into from
Sep 23, 2021

Conversation

killianmuldoon
Copy link
Contributor

@killianmuldoon killianmuldoon commented Sep 13, 2021 •
edited
Loading

What this PR does / why we need it:
This change adds a script that creates a Kind cluster with a local container registry and the correct mounts for using CAPD. This script is mainly a copy of the kind_with_local_registry.sh script made available by the Kind team at https://kind.sigs.k8s.io/docs/user/local-registry/.

This PR is in response to a comment on our current Tilt workflow at #5204

Signed-off-by: killianmuldoon [email protected]

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 13, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 13, 2021
@killianmuldoon killianmuldoon mentioned this pull request Sep 13, 2021
Closed
stmcginnis
stmcginnis suggested changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

Looks pretty good, and matches what I have been doing locally. One small comment to address.

hack/kind_install_for_capd.sh Outdated
@@ -0,0 +1,71 @@
#!/bin/sh

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the copyright header to this script:

https://github.com/kubernetes-sigs/cluster-api/blob/master/hack/ensure-kubectl.sh#L3-L15

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'm wondering about this as this is mostly taken from the KIND script - is it appropriate to add a header to it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

vincepri
vincepri reviewed Sep 15, 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.

Overall looks great!

Should we have some Make commands that runs this script before running tilt up?

hack/kind_install_for_capd.sh Outdated
@@ -0,0 +1,71 @@
#!/bin/sh

Copy link
Member

Choose a reason for hiding this comment

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

Yes

hack/kind_install_for_capd.sh Outdated
@@ -0,0 +1,71 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

Rename this file with - to be consistent with the rest?

hack/kind_install_for_capd.sh Outdated
set -o nounset
set -o pipefail

NAME="capi-test"
Copy link
Member

Choose a reason for hiding this comment

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

Should we allow this to be set outside and default to capi-test?

Copy link
Contributor Author

@killianmuldoon killianmuldoon Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand you correctly that's what's happening here - the --name flag sets the name for the cluster if it's set.

@killianmuldoon killianmuldoon force-pushed the test/add-kind-setup branch 2 times, most recently from a4a567f to 1fdf5a0 Compare September 17, 2021 14:30
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 20, 2021 •
edited
Loading

@killianmuldoon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-verify 1fdf5a0 link /test pull-cluster-api-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

fabriziopandini
fabriziopandini reviewed Sep 22, 2021
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.

/lgtm

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

@killianmuldoon please address linter errors

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2021
@fabriziopandini
Copy link
Member

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 Sep 23, 2021
@k8s-ci-robot k8s-ci-robot merged commit 5efb7d7 into kubernetes-sigs:main Sep 23, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone Sep 23, 2021
@killianmuldoon killianmuldoon mentioned this pull request Sep 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziopandini fabriziopandini fabriziopandini left review comments

@vincepri vincepri vincepri left review comments

@stmcginnis stmcginnis stmcginnis requested changes

@sbueringer sbueringer Awaiting requested review from sbueringer

Assignees

@fabriziopandini fabriziopandini

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.
5 participants
@killianmuldoon @k8s-ci-robot @fabriziopandini @vincepri @stmcginnis