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

feat: update templates to specify control and worker shapes separately #19

Merged
merged 3 commits into from
Mar 23, 2022

Conversation

joekr
Copy link
Member

@joekr joekr commented Mar 5, 2022

What this PR does / why we need it:
While the users can specify the shapes independently we wanted our
defined templates to support this as well. It should make it easier
for our users to quickly define different shapes independent of each
other.

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

@joekr joekr marked this pull request as ready for review March 8, 2022 16:12
@joekr joekr added the enhancement New feature or request label Mar 8, 2022
@joekr joekr self-assigned this Mar 9, 2022
Copy link
Member

@hyder hyder left a comment

Choose a reason for hiding this comment

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

We should also decide how we are going to call the instances: instance, machine, node. Let's pick 1 term and use it consistently. Instance is usually an OCI term whereas I think node is a K8s term and machine is a CAPI term.

docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Show resolved Hide resolved
@joekr joekr force-pushed the issue-15-dynamic-shapes branch 2 times, most recently from 1b27ff8 to 5e70569 Compare March 11, 2022 18:58
@joekr joekr requested review from hyder and Djelibeybi March 11, 2022 19:14
hyder
hyder previously requested changes Mar 15, 2022
Copy link
Member

@hyder hyder left a comment

Choose a reason for hiding this comment

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

A few cosmetic changes

docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
@Djelibeybi
Copy link
Member

I actually got this wrong in my suggestions: the OCI Style Guide requires periods at the end of all sentences, including those in table cells. In this case, this is usually only the content in the description field. Please do add periods at the end of each sentence.

Copy link
Member

@shyamradhakrishnan shyamradhakrishnan left a comment

Choose a reason for hiding this comment

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

please add an output of e2e test and unit test, just for the sake of completeness

docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
@joekr
Copy link
Member Author

joekr commented Mar 15, 2022

Still working on the CCM test, but I think that one is on me. I'm going to rerun after an image update

Summarizing 1 Failure:

[Fail] Workload cluster creation [It] Cloud Provider OCI testing
/Users/jkratzat/projects/cluster-api-provider-oci-joekr/test/e2e/ccm_helpers.go:83

Ran 8 of 16 Specs in 7067.392 seconds
FAIL! -- 7 Passed | 1 Failed | 0 Pending | 8 Skipped
make test
...
?       github.com/oracle/cluster-api-provider-oci      [no test files]
?       github.com/oracle/cluster-api-provider-oci/api/v1beta1  [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/config [no test files]
ok      github.com/oracle/cluster-api-provider-oci/cloud/ociutil        0.866s  coverage: 30.3% of statements
ok      github.com/oracle/cluster-api-provider-oci/cloud/scope  76.402s coverage: 84.0% of statements
?       github.com/oracle/cluster-api-provider-oci/cloud/scope/mocks    [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/compute       [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/compute/mock_compute  [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/identity      [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/identity/mock_identity        [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/networkloadbalancer   [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/networkloadbalancer/mock_nlb  [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/vcn   [no test files]
?       github.com/oracle/cluster-api-provider-oci/cloud/services/vcn/mock_vcn  [no test files]
ok      github.com/oracle/cluster-api-provider-oci/controllers  27.070s coverage: 69.3% of statements

Update

• [SLOW TEST:2001.603 seconds]
Workload cluster creation
/Users/shyaradh/oke/src/github.com/joekr/cluster-api-provider-oci/test/e2e/cluster_test.go:49
  Cloud Provider OCI testing
  /Users/shyaradh/oke/src/github.com/joekr/cluster-api-provider-oci/test/e2e/cluster_test.go:201
------------------------------
SSSSSSSSSSSTEP: Tearing down the management cluster

JUnit report was created: /Users/shyaradh/oke/src/github.com/joekr/cluster-api-provider-oci/_artifacts/junit.e2e_suite.1.xml

Ran 1 of 16 Specs in 2307.319 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 15 Skipped
PASS

@joekr joekr requested review from hyder and Djelibeybi March 15, 2022 23:08
Copy link
Member

@Djelibeybi Djelibeybi left a comment

Choose a reason for hiding this comment

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

Something that I just noticed: our examples don't cover all the options. It would be good if we can provide an example usage of all available parameters. It doesn't all have to be in a single example but rather all parameters must be used at least once across all the provided examples.

docs/src/gs/create-workload-cluster.md Outdated Show resolved Hide resolved
@joekr
Copy link
Member Author

joekr commented Mar 16, 2022

I've created #35 to cover all the examples @Djelibeybi

@joekr joekr force-pushed the issue-15-dynamic-shapes branch 2 times, most recently from 905f41c to 145780e Compare March 17, 2022 12:55
While the users can specify the shapes independently we wanted our
defined templates to support this as well. It should make it easier
for our users to quickly define different shapes independent of each
other.
We are removing this from our main templates otherwise we have to set
a default which users might not set when changing the OCPU and could
cause issues.
@joekr joekr dismissed stale reviews from hyder and Djelibeybi March 23, 2022 16:23

I've addressed the changes. Not sure why it is still blocking the PR

@joekr joekr merged commit 674d84b into oracle:main Mar 23, 2022
@joekr joekr deleted the issue-15-dynamic-shapes branch April 6, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a user I should be able to specify shapes for control and worker nodes separately
4 participants