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

🌱 Export scale e2e test to make it reusable by providers #11333

Merged
merged 1 commit into from
Oct 31, 2024
Merged

🌱 Export scale e2e test to make it reusable by providers #11333

merged 1 commit into from
Oct 31, 2024

Conversation

mboukhalfa
Copy link
Member

Make scale e2e test reusable for CAPM3 and other providers

  • Export ScaleSpec and ScaleSpecInput to allow reuse in other providers.
  • Add PostScaleClusterNamespaceCreated hook, which is called after generating the cluster name and namespace but before applying the template. This enables provider-specific customizations and allows adapting the cluster template accordingly before application such as adding specific cluster endpoints.

Fixes #11332

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 24, 2024
@k8s-ci-robot k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 24, 2024
@mboukhalfa mboukhalfa force-pushed the mboukhalfa/export-scale-e2e-test branch from 7268bf6 to e06306c Compare October 25, 2024 06:27
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 25, 2024
@mboukhalfa mboukhalfa force-pushed the mboukhalfa/export-scale-e2e-test branch 2 times, most recently from 03c3126 to a04ce9f Compare October 25, 2024 12:55
@mboukhalfa
Copy link
Member Author

cc @sbueringer

sbueringer
sbueringer reviewed Oct 31, 2024
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Just a few nits, otherwise ready to merge

test/e2e/scale.go Outdated
clusterNamespace string,
clusterName string,
clusterClassYAML []byte,
clusterTemplateYAML []byte) (clusterClassTemlate []byte, ClusterTemplate []byte)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
clusterTemplateYAML []byte) (clusterClassTemlate []byte, ClusterTemplate []byte)
clusterTemplateYAML []byte) (clusterClassYAML []byte, clusterTemplateYAML []byte)

nit

Let's also add a new line after this line

Copy link
Member Author

Choose a reason for hiding this comment

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

It is complaining that clusterClassYAML redeclared in this bock !

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed that return var name

Copy link
Member

Choose a reason for hiding this comment

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

Ah :)

test/e2e/scale.go Outdated
@@ -568,7 +577,9 @@ func getClusterCreateFn(clusterProxy framework.ClusterProxy) clusterCreator {
}
}

func createClusterWorker(ctx context.Context, clusterProxy framework.ClusterProxy, inputChan <-chan string, resultChan chan<- workResult, wg *sync.WaitGroup, defaultNamespace string, deployClusterInSeparateNamespaces bool, baseClusterClassYAML, baseClusterTemplateYAML []byte, create clusterCreator) {
type PostScaleClusterNamespaceCreated func(clusterProxy framework.ClusterProxy, clusterNamespace string, clusterName string, clusterClassYAML []byte, clusterTemplateYAML []byte) (clusterClassTemlate []byte, ClusterTemplate []byte)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type PostScaleClusterNamespaceCreated func(clusterProxy framework.ClusterProxy, clusterNamespace string, clusterName string, clusterClassYAML []byte, clusterTemplateYAML []byte) (clusterClassTemlate []byte, ClusterTemplate []byte)
type PostScaleClusterNamespaceCreated func(clusterProxy framework.ClusterProxy, clusterNamespace string, clusterName string, clusterClassYAML []byte, clusterTemplateYAML []byte) (clusterClassYAML []byte, clusterTemplateYAML []byte))

Copy link
Member Author

Choose a reason for hiding this comment

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

removed return var names

test/e2e/scale.go Outdated Show resolved Hide resolved
@sbueringer sbueringer changed the title 🌱 Make scale e2e test reusable for CAPM3 and other providers 🌱 Export scale e2e test to make it reusable outside of core Cluster API Oct 31, 2024
@sbueringer sbueringer changed the title 🌱 Export scale e2e test to make it reusable outside of core Cluster API 🌱 Export scale e2e test to make it reusable by providers Oct 31, 2024
@mboukhalfa
Make scale e2e test reusable for CAPM3 and other providers
241b8e4 
- Export ScaleSpec and ScaleSpecInput to allow reuse in other providers.
- Add PostScaleClusterNamespaceCreated hook to be called after generating cluster name and namespace but before applying the template.

Signed-off-by: Mohammed Boukhalfa <mohammed.boukhalfa@est.tech>
@mboukhalfa mboukhalfa force-pushed the mboukhalfa/export-scale-e2e-test branch from a04ce9f to 241b8e4 Compare October 31, 2024 10:54
@mboukhalfa mboukhalfa requested a review from sbueringer October 31, 2024 10:55
@mboukhalfa
Copy link
Member Author

/assign @mquhuy
cc @adilGhaffarDev
can follow up if more changes needed

@k8s-ci-robot
Copy link
Contributor

@mboukhalfa: GitHub didn't allow me to assign the following users: mquhuy.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @mquhuy
cc @adilGhaffarDev
can follow up if more changes needed

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-sigs/prow repository.

@adilGhaffarDev
Copy link
Contributor

/assign @mquhuy

@k8s-ci-robot
Copy link
Contributor

@adilGhaffarDev: GitHub didn't allow me to assign the following users: mquhuy.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @mquhuy

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-sigs/prow repository.

@mboukhalfa mboukhalfa mentioned this pull request Oct 31, 2024
Merged
@sbueringer sbueringer added the area/e2e-testing Issues or PRs related to e2e testing label Oct 31, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Oct 31, 2024
@sbueringer
Copy link
Member

Thx!
/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 Oct 31, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e2ae4eab6908986212636d33e70ca425f2395cda

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 Oct 31, 2024
@k8s-ci-robot k8s-ci-robot merged commit 9b0190f into kubernetes-sigs:main Oct 31, 2024
19 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.9 milestone Oct 31, 2024
@Sunnatillo Sunnatillo mentioned this pull request Nov 19, 2024
Closed
54 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@richardcase richardcase Awaiting requested review from richardcase

@sbueringer sbueringer Awaiting requested review from sbueringer

Assignees

@sbueringer sbueringer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-testing Issues or PRs related to e2e testing 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
v1.9
Development

Successfully merging this pull request may close these issues.

[e2e tests] Make the scaleSpec Public and Add Hook for Reusability
4 participants
@mboukhalfa @k8s-ci-robot @adilGhaffarDev @sbueringer