-
Notifications
You must be signed in to change notification settings - Fork 203
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 support for kubemark #220
Add support for kubemark #220
Conversation
/retest |
2 similar comments
/retest |
/retest |
44ae62f
to
b76f6a7
Compare
/retest |
config/0000_00_cluster-version-operator_01_clusteroperator.crd.yaml
Outdated
Show resolved
Hide resolved
|
||
.PHONY: build-integration | ||
build-integration: ## Build integration test binary | ||
@echo -e "\033[32mBuilding integration test binary...\033[0m" | ||
mkdir -p bin | ||
$(DOCKER_CMD) go build $(GOGCFLAGS) -o bin/integration github.com/openshift/machine-api-operator/test/integration | ||
|
||
.PHONY: deploy-kubemark | ||
deploy-kubemark: | ||
kustomize build config | kubectl apply -f - |
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.
Is kustomize necessary here?
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.
Suggesting to run it without the Makefile?
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.
No, I'm suggesting to not use kustomize. Not sure what it's doing.
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.
Just taking all manifests and pumping them into a single stream. Running just one command to deploy all manifests at once.
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 believe kubectl can do that too; just do kubectl create -f ./
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.
The kustomize
is (among other things) changing default namespace and patching manifests. Something simple kubectl
can not do.
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.
cluster-api, along with kubebuilder and so operator-sdk use kustomize. I'm in favour of using it here for manifest generation and customization
just a few nits, overall lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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 |
b76f6a7
to
352d4f9
Compare
/test integration |
fi | ||
|
||
status=$(kubectl get node $node -o json | jq '.status.conditions[] | select(.type=="Ready") | .status' --raw-output) | ||
if [ $status != "Unknown" ]; then |
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 invert it to increase readability?
if [ $status == "Unknown" ]; then
echo "Deleting node $node"
kubectl delete node $node
fi
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 see any improvement in readibility compared to the current implementation. Can you more elaborate on that?
352d4f9
to
86d5e88
Compare
Just rebasing |
/lgtm |
/retest |
86d5e88
to
32314fe
Compare
New changes are detected. LGTM label has been removed. |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@@ -29,6 +29,8 @@ const ( | |||
LibvirtProvider = Provider("libvirt") |
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 should add this case into config_test
/retest |
32314fe
to
86992f9
Compare
New changes are detected. LGTM label has been removed. |
Pulling openshift/cluster-api-actuator-pkg#36 to see more from the failing autoscaler scales out test. |
/test e2e-aws |
…when-placement-is-nil Check if placement is not nil before accessing AvailabilityZone field
Still hacky since there is nothing rending the kubemark RBAC rules right know and we still don't have our own kubemark version of cloud controller manager that would remove NotReady nodes.