-
Notifications
You must be signed in to change notification settings - Fork 115
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
Add kind-dev command to start a kind dev cluster #338
base: main
Are you sure you want to change the base?
Conversation
scripts/run-dev.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think run-dev
is a very descriptive name for this file. Could you please rename it to something like setup-kind-cluster
or create-dev-cluster
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya , renamed to create-dev-cluster
scripts/run-dev.sh
Outdated
ensure_cluster() { | ||
info_msg "Creating KIND cluster ..." | ||
setup_kind_cluster "$CLUSTER_NAME" "$CONTROLLER_NAMESPACE" | ||
|
||
info_msg "Installing CRDs , common and RBAC manifest..." | ||
install_crd_and_rbac "$CONTROLLER_NAMESPACE" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have an ensure_cluster
method in another file that achieves a similar premise - https://github.com/aws-controllers-k8s/test-infra/blob/main/scripts/run-e2e-tests.sh#L31-L44
This method also uses the config YAML to determine the name, in case the user wants to use a statically named cluster. Rather than redefining this method here, maybe you could modify that one to have a similar effect and then import and use it in this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor here refactor ensure_cluster
scripts/run-dev.sh
Outdated
ensure_aws_credentials | ||
ensure_cluster | ||
local kubeconfig_path="$ROOT_DIR/build/clusters/$CLUSTER_NAME/kubeconfig" | ||
info_msg "Before running the controller, you need kubeconfig and aws credentials." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Probably don't need this line of explanation. I think we can assume anyone using this method already knows this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
scripts/run-dev.sh
Outdated
info_msg "if you run the controller from your code editor/IDE, you can set the following environment variables:" | ||
echo "KUBECONFIG=$kubeconfig_path" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already called out from inside the setup_kind_cluster
method. See https://github.com/aws-controllers-k8s/test-infra/blob/main/scripts/kind.sh#L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the source command set the kubeconfig in current terminal only, if starting the controller from goland or vscode, we have to set KUBECONFIG into them. but you're right, better to mention in document rather than here.
scripts/run-dev.sh
Outdated
info_msg "After executing the above source command to set the kubeconfig to environment, " | ||
info_msg "run the following command to start the controller from the controller repo:" | ||
echo "" | ||
echo "go run ./cmd/controller/main.go --aws-region eu-central-1 --log-level debug --enable-development-logging" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Instead of hardcoding eu-central-1
, get the region from the config YAML (like this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, good stuff!
nit: can you please update the title and description of PR to reflect your latest changes? They will be part of your git commit
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, Julian-Chu 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 |
@a-hilaly thanks for review, just see your comments. the title and description have been updated! |
scripts/run-e2e-tests.sh
Outdated
_ensure_existing_context | ||
else | ||
local cluster_name=$(get_cluster_name) | ||
local controller_install=${2:-true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please move all of the positional arguments into local variables at the top of this method, so that it's clear what the inputs are. Then you can copy them into their respective variables further down in the method as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved 4c321c2
scripts/run-e2e-tests.sh
Outdated
else | ||
info_msg "Testing connection to existing cluster ..." | ||
_ensure_existing_context | ||
info_msg "Installing CRDs , common and RBAC manifest..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info_msg "Installing CRDs , common and RBAC manifest..." | |
info_msg "Installing CRD and RBAC manifests..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 4ed9f07
Issues go stale after 180d of inactivity. |
Stale issues rot after 60d of inactivity. |
@Julian-Chu Are we ready to ship this? |
Just want to double check that this is not gonna break the existing testing rule |
Issues go stale after 180d of inactivity. |
Stale issues rot after 60d of inactivity. |
Description of changes:
Add
make kind-dev SERVICE=$SERVICE
to setup a cluster for development or debug quickly.the command will execute the following actions:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.