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

fix: set defaults for AWS CP and Worker instanceType #504

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

supershal
Copy link
Contributor

Moved d2iq-labs#51
Addressed the pending review comments
What problem does this PR solve?:

  • sets defaults for aws nodes CP: m5.xlarge and Workers: m5.2xlarge
  • sets PLACEHOLDER value as defaults in AWSMachineTemplates because instanceType field is required by CAPA.
  • sets instance type as required field in the schema since CAREN will be using the default if not provided by the user.
    update existing unit tests to use default schema
    NOTE
    Once this PR is finalized, I will create follow up PR to set instance profile and some other changes in the API types that would ensure that it creates variable schema from the object instead of creating new one.

Which issue(s) this PR fixes:
Fixes # #485

How Has This Been Tested?:

Tried to test locally using make e2e-test E2E_LABEL='provider:AWS && cni:Cilium' but local environment timed outs during initializations. Looking into fixing local environment (colima +docker +kind on mac)

Tested manually be creating AWS cluster.

aws-quick-start cluster class has following

 instanceType:
                      default: m5.xlarge
                      description: The AWS instance type to use for the cluster Machines
                      type: string
                  required:
                  - instanceType

and workers

                instanceType:
                  default: m5.2xlarge
                  description: The AWS instance type to use for the cluster Machines
                  type: string
              required:
              - instanceType

Created AWS cluster without adding instanceType variable, CAREN patched AWSMachineTemplate with default instanceType.

** Special notes for your reviewer:**

The approach taken in this PR will be used as guide to set default for other variables.

@supershal supershal self-assigned this Apr 11, 2024
@supershal supershal changed the title Shalin/ncn default instance type fix: set defaults for AWS CP and Worker instanceType Apr 11, 2024
@github-actions github-actions bot added the fix label Apr 11, 2024
@jimmidyson jimmidyson merged commit 64e6e72 into main Apr 15, 2024
13 of 16 checks passed
@jimmidyson jimmidyson deleted the shalin/ncn-default_instance_type branch April 15, 2024 16:44
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
jimmidyson pushed a commit that referenced this pull request Apr 15, 2024
Moved:
d2iq-labs#54

**What problem does this PR solve?:**
This is a stacked PR:
#504

sets defaults for aws instance profiles CP:
`control-plane.cluster-api-provider-aws.sigs.k8s.io` and Workers:
`nodes.cluster-api-provider-aws.sigs.k8s.io`
removes instanceProfile field in AWSMachineTemplates
**Which issue(s) this PR fixes:**
Fixes #

**How Has This Been Tested?:**

Tested manually be creating AWS cluster.

Created AWS cluster without adding instanceProfile variable, CAREN
patched AWSMachineTemplate with default instanceProfile.

**Special notes for your reviewer:**

Looking into ways to unit test the defaults.
jimmidyson pushed a commit that referenced this pull request Apr 15, 2024
🤖 I have created a release *beep* *boop*
---


## 0.7.0 (2024-04-15)

<!-- Release notes generated using configuration in .github/release.yaml
at main -->

## What's Changed
### Exciting New Features 🎉
* feat: Sync up from d2iq-labs fork by @jimmidyson in
#489
* feat: set default instance profile for AWS CP and worker nodes by
@supershal in
#506
### Fixes 🔧
* fix: set defaults for AWS CP and Worker instanceType by @supershal in
#504
### Other Changes
* build: Remove unused tool crane by @jimmidyson in
#459
* ci: Add govulncheck check by @jimmidyson in
#461
* ci: Remove auto-approve PR steps by @jimmidyson in
#462
* build: Tidy up examples sync script by @jimmidyson in
#458
* test: Remove redundant test case from httpproxy handler by
@dlipovetsky in
#463
* ci: Fix pages workflow concurrency by @jimmidyson in
#493
* refactor: Replace direct usage of CAAPH API with vendored types by
@jimmidyson in
#492
* refactor: Update module paths to use nutanix-cloud-native GH org by
@jimmidyson in
#494
* build: Remove unused capbk and capd hack modules by @jimmidyson in
#496
* docs: add pull request template for the repository by @supershal in
#502
* docs: Add file extension to containerd-metrics doc by @dlipovetsky in
#503
* build: set dockerhub credentials for Nutanix examples by @dkoshkin in
#501


**Full Changelog**:
v0.6.0...v0.7.0

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants