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

Batch UpdateComputeEnvironment will not update MinvCpus to zero #1414

Closed
3 tasks done
cgmartin opened this issue Sep 11, 2021 · 4 comments
Closed
3 tasks done

Batch UpdateComputeEnvironment will not update MinvCpus to zero #1414

cgmartin opened this issue Sep 11, 2021 · 4 comments
Labels
bug This issue is a bug. service-api This issue is due to a problem in a service API, not the SDK implementation.

Comments

@cgmartin
Copy link

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
Batch UpdateComputeEnvironment will not update MinvCpus to zero.

Version of AWS SDK for Go?
AWS version: 1.8.1

Version of Go (go version)?
Go version: go1.16.7

To Reproduce (observed behavior)

input := &batch.UpdateComputeEnvironmentInput{
	ComputeEnvironment: computeEnv.ComputeEnvironmentArn,
	ComputeResources: &batchTypes.ComputeResourceUpdate{
		MinvCpus:     0,
		MaxvCpus:     16,
		DesiredvCpus: 4,
	},
	State: batchTypes.CEStateDisabled,
}
output, err := s.BatchClient.UpdateComputeEnvironment(ctx, input)

// Results in the following request body to AWS backend:
// {"computeEnvironment":"arn:aws:batch:us-east-1:...","computeResources":{"desiredvCpus":4,"maxvCpus":16},"state":"DISABLED"}

The "minvCpus" property is not sent.

Expected behavior

I expect to be able to change the minvCpus value to 0 with an UpdateComputeEnvironment call.

Additional context

Serializers are currently implemented to exclude zero values for MinvCpus, MaxvCpus and DesiredvCpus, see

if v.DesiredvCpus != 0 {
ok := object.Key("desiredvCpus")
ok.Integer(v.DesiredvCpus)
}
if v.MaxvCpus != 0 {
ok := object.Key("maxvCpus")
ok.Integer(v.MaxvCpus)
}
if v.MinvCpus != 0 {
ok := object.Key("minvCpus")
ok.Integer(v.MinvCpus)
}

@cgmartin cgmartin added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 11, 2021
@jasdel
Copy link
Contributor

jasdel commented Nov 3, 2021

Thanks for reporting this bug @cgmartin. We're investigating the best way to handle this issue. We think this is similar to the modeling issue the SDK had with Amazon EC2's API model, e.g. #1107.

Similar to the EC2 issue, it looks like the fields should of been modeled as boxed (aka nullable), instead of unboxed, (aka non-nullable). We're investigating if there is a way to fix this issue without making a breaking change to the batch module. The ec2 module had to have a breaking change because there was no other workaround. Its possible this will be needed for batch as well. We're investigating if we can limit the scope of this issue, and address it in the future for other APIs.

@jasdel jasdel added service-api This issue is due to a problem in a service API, not the SDK implementation. and removed needs-triage This issue or PR still needs to be triaged. labels Nov 3, 2021
@cgmartin
Copy link
Author

cgmartin commented Nov 3, 2021

Thank you for reviewing this. FWIW based on the API call, where fields are intended to only be included if they need to change, I agree that it should be modeled as boxed (which was my approach in my local fix #1466).

And I empathize about the potential breaking change. My expertise with golang is low to confidently suggest a non-breaking path forward. The only idea I have would be to include a completely separate UpdateComputeEnvironmentV2() function and UpdateComputeEnvironmentInputV2 / ComputeResourceUpdateV2 structs that users could migrate to, though that could potentially cause confusion over the long term.

@jasdel
Copy link
Contributor

jasdel commented Mar 28, 2022

service/[email protected] has been updated with a fix that corrects the representation of the members correctly.

go get github.com/aws/aws-sdk-go-v2/service/[email protected]

This is a breaking change though. The breaking change was accepted because the API operation could not be used correctly with the members modeled as unboxed (aka value) types. The update changes the members to boxed (aka pointer) types so that the zero value of the members can be handled correctly by the SDK and service.

Your application will fail to compile with the updated module. To workaround this you'll need to update your application to use pointer types for the members impacted.

@jasdel jasdel closed this as completed Mar 28, 2022
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. service-api This issue is due to a problem in a service API, not the SDK implementation.
Projects
None yet
Development

No branches or pull requests

2 participants