-
Notifications
You must be signed in to change notification settings - Fork 82
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 defaults v mutually exclusive args conflict #813
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
3 similar comments
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
441e75a
to
379acd1
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks right and good and nicely self-contained to me. As a co-author though, I'll leave the official green-ticking to Daniel.
I think this approach is all correct, but I think we should probably remove all default values in the schema. We never want the SDK providing a default value rather - we're rather the SDK passes through the lack of value and let the nodejs implementation calculate the defaults provider-side. |
Previously a default for nodeRootVolumeSize was set in the schema (and therefore the SDKs), which would mean the value was always set. This would cause the mutual exclusion check for nodeGroupOptions to always fail if nodeGroupOptions was ever supplied. This is not needed, as the platform itself defaults the value.
a9d4bbf
to
0171975
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
eff99fa
to
fc163b3
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
fc163b3
to
d3e8283
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
PR #641 added a default value to the Cluster argument
nodeRootVolumeSize
, which causes all the generated SDKs to give this field a default value if not supplied. However,pulumi-eks/nodejs/eks/cluster.ts
Line 231 in aab0ed9
nodeGroupOptions
or individual arguments includingnodeRootVolumeSize
are provided, and not both. SincenodeRootVolumeSize
always has a value, this means that if you supplynodeGroupOptions
, it will fail to create your cluster.On the basis that the platform applies the default and the SDK does not need to, this PR removes the default for that specific field from the schema.
Fixes #643.