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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃尡 Enable experimental features in e2e testing #787

Merged
merged 1 commit into from Jul 15, 2020
Merged

馃尡 Enable experimental features in e2e testing #787

merged 1 commit into from Jul 15, 2020

Conversation

devigned
Copy link
Contributor

What this PR does / why we need it:

The current E2E tests do not enable experimental features. This PR enables ENV var substitution for known feature flags and populates env vars for the E2E tests with true, enabling the features.

related: kubernetes-sigs/cluster-api#3325

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Enable env substitution for experimental features and enable them in the E2E tests

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 14, 2020
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 14, 2020
@devigned
Copy link
Contributor Author

/hold

I'm cleaning up some of the extraneous flags and code to replace flags.

@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 14, 2020
@CecileRobertMichon
Copy link
Contributor

I'm not sure kubernetes-sigs/cluster-api#3325 made it in the rc-1 release so you might not be able to use this yet

@devigned
Copy link
Contributor Author

@CecileRobertMichon, great eyes! I opened up #788 to help address that. I'll rebase this on master after #788 is pulled in.

$ curl -Ls https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.3.7/cluster-api-components.yaml | grep feature
        - --feature-gates=MachinePool=${EXP_MACHINE_POOL:=false},ClusterResourceSet=${EXP_CLUSTER_RESOURSE_SET:=false}
        - --feature-gates=MachinePool=${EXP_MACHINE_POOL:=false},ClusterResourceSet=${EXP_CLUSTER_RESOURSE_SET:=false}
                  featureGates:
                  featureGates:
                          featureGates:
                          featureGates:
                      featureGates:

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 14, 2020
devigned
devigned commented Jul 14, 2020
View reviewed changes
Makefile
@@ -387,18 +383,12 @@ create-management-cluster: $(KUSTOMIZE) $(ENVSUBST)
kubectl wait --for=condition=Available --timeout=5m apiservice v1beta1.webhook.cert-manager.io

# Deploy CAPI
kubectl apply -f https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.3.7/cluster-api-components.yaml
curl -sSL https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.3.7/cluster-api-components.yaml | $(ENVSUBST) | kubectl apply -f -
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that envsubst is called, if ENV vars are set (EXP_MACHINE_POOL=true, etc) the flags will be set to true.

Copy link
Contributor

Choose a reason for hiding this comment

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

fyi @detiber @randomvariable you probably need to do the same in CAPA/CAPV

@CecileRobertMichon
Copy link
Contributor

/hold

there is some discussion around possibly reverting the CAPI commit because of kubernetes-sigs/cluster-api#3339

@CecileRobertMichon
Copy link
Contributor

ok nvm we are not reverting

/hold cancel

@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 14, 2020
devigned
devigned commented Jul 14, 2020
View reviewed changes
.tiltignore
@@ -0,0 +1 @@
templates
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This stops tilt from doing a full rebuild when a flavor is triggered.

@CecileRobertMichon CecileRobertMichon mentioned this pull request Jul 14, 2020
Closed
@devigned
Copy link
Contributor Author

/hold

for a little bit more testing locally

@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 15, 2020
sedefsavas
sedefsavas reviewed Jul 15, 2020
View reviewed changes
config/manager/manager.yaml Outdated Show resolved Hide resolved
config/manager/manager_auth_proxy_patch.yaml Outdated Show resolved Hide resolved
config/webhook/manager_webhook_patch.yaml Outdated Show resolved Hide resolved
CecileRobertMichon
CecileRobertMichon approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/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 Jul 15, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 15, 2020
@devigned
Copy link
Contributor Author

Looking good, but ran into a bug when testing templates (#791). I'll follow up with a fix.

EXP_MACHINE_POOL=true EXP_AKS=true make tilt-up

image

@devigned
Copy link
Contributor Author

/hold cancel
/assign @CecileRobertMichon

@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 15, 2020
@k8s-ci-robot k8s-ci-robot merged commit 82b903d into kubernetes-sigs:master Jul 15, 2020
@k8s-ci-robot k8s-ci-robot added this to the v0.4.7 milestone Jul 15, 2020
@devigned devigned deleted the e2e-exp branch July 15, 2020 20:04
@ams0 ams0 mentioned this pull request Nov 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sedefsavas sedefsavas sedefsavas left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon approved these changes

@justaugustus justaugustus Awaiting requested review from justaugustus

Assignees

@CecileRobertMichon CecileRobertMichon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v0.4.7
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@devigned @CecileRobertMichon @k8s-ci-robot @sedefsavas