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.

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

Refactoring the provider to unit test create cluster #5259

Merged
merged 16 commits into from
May 18, 2022

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented May 16, 2022

Description

Ground work for #5015.

What happens in this PR?

  1. Renamed Provider to AWSProvider since it provides AWS functionality
  2. ClusterProvider is no longer a KubeProvider. It has been extracted into a separate part so it can be mocked more easily
  3. doClusterCreate now gets the ctl passed in from the outside so we can mock it
  4. Several small refactors and test changes for mock calls

Integration test run https://github.com/weaveworks/eksctl-ci/actions/runs/2330469236

edit: New test run https://github.com/weaveworks/eksctl-ci/actions/runs/2332032290

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@Skarlso Skarlso added the skip-release-notes Causes PR not to show in release notes label May 16, 2022
@Skarlso Skarlso force-pushed the unit_test_create_cluster branch from 4dbbfc2 to e4b8ee1 Compare May 16, 2022 07:26
@Skarlso Skarlso force-pushed the unit_test_create_cluster branch from 74d3ca5 to c264a78 Compare May 16, 2022 13:00
@Skarlso Skarlso force-pushed the unit_test_create_cluster branch from c264a78 to 0899701 Compare May 16, 2022 13:08
@Skarlso
Copy link
Contributor Author

Skarlso commented May 17, 2022

Integration tests passed except for a single test which timed out on TLS. That appears to be a fluke from AWS though. I'm running it locally to verify that.

Edit: Works:

Ran 3 of 3 Specs in 156.705 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestBeforeActive (156.71s)
PASS

Copy link
Contributor

@cPu1 cPu1 left a comment

Choose a reason for hiding this comment

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

Just one comment about considering embedding KubeProvider, but otherwise LGTM.

My deadline-based Context PR will result in conflicts, so I'll wait for this to be merged before merging mine.

pkg/eks/api.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Himangini Himangini left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@Skarlso Skarlso merged commit 3afdd26 into main May 18, 2022
@Skarlso Skarlso deleted the unit_test_create_cluster branch May 18, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-release-notes Causes PR not to show in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants