-
Notifications
You must be signed in to change notification settings - Fork 50
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: support implicitGlobalRegion
#1335
Conversation
A new generated diff is ready to view. |
A new generated diff is ready to view. |
…plicit-global-region
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
A new generated diff is ready to view. |
A new generated diff is ready to view. |
A new generated diff is ready to view. |
Quality Gate passedIssues Measures |
A new generated diff is ready to view. |
Affected ArtifactsChanged in size
|
@InternalSdkApi | ||
public data class PartitionConfig( | ||
public data class PartitionConfig @JvmOverloads constructor( | ||
public val name: String? = null, | ||
public val dnsSuffix: String? = null, | ||
public val dualStackDnsSuffix: String? = null, | ||
public val supportsFIPS: Boolean? = null, | ||
public val supportsDualStack: Boolean? = null, | ||
public val implicitGlobalRegion: String? = null, | ||
) { |
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.
correctness: I think if you also add a 2nd constructor it might save us from having to break the API
public data class PartitionConfig (
public val name: String? = null,
public val dnsSuffix: String? = null,
public val dualStackDnsSuffix: String? = null,
public val supportsFIPS: Boolean? = null,
public val supportsDualStack: Boolean? = null,
public val implicitGlobalRegion: String? = null,
) {
@JvmOverloads
public constructor (
name: String? = null,
dnsSuffix: String? = null,
dualStackDnsSuffix: String? = null,
supportsFIPS: Boolean? = null,
supportsDualStack: Boolean? = null,
) : this(
name,
dnsSuffix,
dualStackDnsSuffix,
supportsFIPS,
supportsDualStack,
null,
)
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.
Which specific API break are you concerned about? I added @JvmOverloads
so there are new constructors generated for each parameter. I think that nullifies the API "break" reported by the validator
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.
I don't really want to just add a second constructor because:
- This is
@InternalSdkApi
- This will continue being an issue as long as we use data classes. But I think
@JvmOverloads
help with this. I made an internal issue SDK-KT-215 to research adding this to all our data classes
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.
Oh okay, nvm then. I was worried about adding unnecessary ones and the ones from line 52 & 53. You're also right that it's internal anyway
Adds a new partitions property
implicitGlobalRegion
note: MRAP e2e tests had to be disabled for this to pass CI. Check out internal issue SDK-KT-214 for more context
Issue #
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.