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

compiler break: change LaunchTemplateConfiguration.SetDefaultVersion from bool to *bool #2838

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

lucix-aws
Copy link
Contributor

@lucix-aws lucix-aws commented Oct 15, 2024

We are implementing this compiler break to fix abjectly broken behavior due to a service modeling issue. See the changelog entry for more information.

Closes #2734

@lucix-aws lucix-aws requested a review from a team as a code owner October 15, 2024 20:32
@Madrigal
Copy link
Contributor

I'm a bit concerned about the behavior mentioned at aws/aws-sdk#736

when the value is not explicitly sent, the service assigns a true default value for it.

With this change, we will always set it to false. Not sure what the service implications are for it, maybe it means that when you create new distributions you no longer have a default value until you set one explicitly?

@lucix-aws
Copy link
Contributor Author

I'm a bit concerned about the behavior mentioned at aws/aws-sdk#736

when the value is not explicitly sent, the service assigns a true default value for it.

With this change, we will always set it to false. Not sure what the service implications are for it, maybe it means that when you create new distributions you no longer have a default value until you set one explicitly?

I just had a conversation with the service team about this and they've confirmed that the tri-state capability is important. So I'm going to switch this to removing @default instead of backfilling @required. It's a compiler break rather than a runtime break, basically.

Going forward we should take care to try to pin down service teams to get an idea of what the input surface should be, even if we can't get them to prioritize a fix.

@lucix-aws lucix-aws force-pushed the fix-imagebuildersetdefaultversion branch from 8fcb670 to 3ddce7d Compare October 15, 2024 21:41
"com.amazonaws.emrserverless#WorkerCounts"));
"com.amazonaws.emrserverless#WorkerCounts"),
serviceToShapeIds("com.amazonaws.imagebuilder#imagebuilder",
// https://github.com/aws/aws-sdk-go-v2/issues/2734
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for actually putting the issue and ticket here, otherwise we'd be like

"why do we need this customization?"
"🤷"

Curious as to what happens when/if it gets fixed by the service. What happens with this customization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing, it's a no-op for ShapeBuilder.removeTrait:

    public B removeTrait(ShapeId traitId) {
        if (this.traits.hasValue()) {
            ((Map)this.traits.get()).remove(traitId);
        }

        return this;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"why do we need this customization?"

Currently asking myself this for every single other thing on this list, in fact!

@lucix-aws lucix-aws changed the title always serialize imagebuilder LaunchTemplateConfiguration.SetDefaultVersion compiler break: change LaunchTemplateConfiguration.SetDefaultVersion from bool to *bool Oct 15, 2024
@lucix-aws lucix-aws force-pushed the fix-imagebuildersetdefaultversion branch from 3ddce7d to 94e49e8 Compare October 15, 2024 23:16
@lucix-aws lucix-aws merged commit 09f35b4 into main Oct 16, 2024
13 checks passed
@lucix-aws lucix-aws deleted the fix-imagebuildersetdefaultversion branch October 16, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIGRATION ISSUE: imagebuilder.LaunchTemplateConfiguration.SetDefaultVersion not sending false value
3 participants