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

split out AKS E2E source files #2909

Merged
merged 1 commit into from
Dec 10, 2022
Merged

split out AKS E2E source files #2909

merged 1 commit into from
Dec 10, 2022

Conversation

jackfrancis
Copy link
Contributor

@jackfrancis jackfrancis commented Dec 7, 2022 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR splits up all of the various AKS E2E test foo in test/e2e/aks.go into a bunch of discrete files to maintain specific tests.

The practical impetus of this is to use a newer SDK version of the AKS API which has a more updated representation of configuration. The existing AKS "get me the latest version" foo, unfortunately, depends on an older version of the SDK which is not forward-compatible. Attempting to do that surgery at this time is more work than justifies (IMO).

This is also (IMO) a more maintainable path.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

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:

NONE

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 7, 2022
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 7, 2022
@jackfrancis
Copy link
Contributor Author

/assign @nawazkh

@jackfrancis
Copy link
Contributor Author

/retest

@jackfrancis jackfrancis requested a review from nawazkh December 7, 2022 17:38
@nawazkh
Copy link
Member

nawazkh commented Dec 7, 2022

Looks like timeout errors on the PR tests, thanks for putting this together!

[FAILED] Timed out after 1800.001s.
[1251](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/2909/pull-cluster-api-provider-azure-e2e/1600296720208498688#1:build-log.txt%3A1251)
  Timed out waiting for 3 control plane machines to exist
[1252](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/2909/pull-cluster-api-provider-azure-e2e/1600296720208498688#1:build-log.txt%3A1252)
  Expected
[1253](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/2909/pull-cluster-api-provider-azure-e2e/1600296720208498688#1:build-log.txt%3A1253)
      <int>: 2
[1254](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/2909/pull-cluster-api-provider-azure-e2e/1600296720208498688#1:build-log.txt%3A1254)
  to equal
[1255](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/2909/pull-cluster-api-provider-azure-e2e/1600296720208498688#1:build-log.txt%3A1255)
      <int>: 3
[1256](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/2909/pull-cluster-api-provider-azure-e2e/1600296720208498688#1:build-log.txt%3A1256)
  In [It] at: /home/prow/go/pkg/mod/sigs.k8s.io/cluster-api/[email protected]/framework/controlplane_helpers.go:117 @ 12/07/22 01:58:59.972

CecileRobertMichon
CecileRobertMichon reviewed Dec 7, 2022
View reviewed changes
test/e2e/aks_autoscaler.go
Copyright 2022 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of having a bunch of aks_ files, what are your thoughts on adding an aks sub folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a slightly more advanced operation but if that has more long-term maintenance benefits, happy to do that.

@mboersma @nojnhuh thoughts here?

Copy link
Contributor

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 strong reasons to prefer one option over the other. I can see where a subdirectory would keep the file tree a bit tidier, but readability in azure_test.go might take a marginal hit if the AKS test specs need a package prefix. In that case if we had something like AzureLBSpec and aks.AutoscaleSpec used similarly, I might assume at first glance that they're more different conceptually than they actually are based on one being in a sub-package. That's quite a bit of "reading between the lines" though.

Overall I think I'm leaning slightly toward keeping the AKS files in this existing directory and not putting them in their own.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify I don't have any strong arguments for adding a folder, just pointing it out since it jumped at me when seeing all the different files with the same prefix. Either option is fine with me.

@jackfrancis jackfrancis added this to the v1.7 milestone Dec 7, 2022
@jackfrancis
Copy link
Contributor Author

/retest

1 similar comment
@nawazkh
Copy link
Member

nawazkh commented Dec 7, 2022

/retest

@nawazkh
Copy link
Member

nawazkh commented Dec 8, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2022
@nawazkh
Copy link
Member

nawazkh commented Dec 8, 2022

/approve

1 similar comment
@jackfrancis
Copy link
Contributor Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, nawazkh

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 Dec 8, 2022
@nojnhuh
Copy link
Contributor

nojnhuh commented Dec 9, 2022

/retest

1 similar comment
@jackfrancis
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 6b80e1b into kubernetes-sigs:main Dec 10, 2022
@jackfrancis jackfrancis mentioned this pull request Feb 28, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nojnhuh nojnhuh nojnhuh left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@devigned devigned Awaiting requested review from devigned

@Jont828 Jont828 Awaiting requested review from Jont828

@nawazkh nawazkh Awaiting requested review from nawazkh

Assignees

@nawazkh nawazkh

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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
5 participants
@jackfrancis @nawazkh @k8s-ci-robot @nojnhuh @CecileRobertMichon