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

Bug(aws-sdk-go-v2/service/athena): Breaking change released as a minor version bump #2844

Closed
3 tasks done
erezrokah opened this issue Oct 21, 2024 · 9 comments
Closed
3 tasks done
Assignees
Labels
bug This issue is a bug. potential-regression Marking this issue as a potential regression to be checked by team member service-api This issue is due to a problem in a service API, not the SDK implementation.

Comments

@erezrokah
Copy link

erezrokah commented Oct 21, 2024

Acknowledgements

Describe the bug

The latest release (v1.48.0) of the Athena SDK removed some fields from types.DataCatalog.

The commit is here. Fields removed are ConnectionType, Error, Status

This is a breaking change so it should be released as v2 I think (unless this SDK doesn't follow SemVer), or at explain the reasoning for the change and how to migrate from it.

Regression Issue

  • Select this option if this issue appears to be a regression.

Expected Behavior

Breaking changes should be released in major version bumps, or at least have an descriptive changelog message with a migration path

Current Behavior

Breaking change released in a minor bump without explaining how to handle the change

Reproduction Steps

Update from Athena SDK v1.47.2 to v1.48.0

Possible Solution

When fields are removed or have their types changed (i.e. a breaking change is done), document how to handle the change and why it was done

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

github.com/aws/aws-sdk-go-v2/service/athena

Compiler and Version used

go version go1.23.1 darwin/arm64

Operating System and version

macOS 15.0.1

@erezrokah erezrokah added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 21, 2024
@github-actions github-actions bot added the potential-regression Marking this issue as a potential regression to be checked by team member label Oct 21, 2024
@lucix-aws
Copy link
Contributor

related: #2549

@lucix-aws
Copy link
Contributor

lucix-aws commented Oct 21, 2024

Will need to confirm internally to see whether this was intentional.

For context, service API changes happen automatically and are separate from general SDK development. Service teams own API models that they push to our release system to be part of daily releases. They are NOT supposed to make breaking changes like this, we can and do push back when they attempt to, but we ultimately do not have the power to prevent it from happening if the team escalates.

@lucix-aws lucix-aws added service-api This issue is due to a problem in a service API, not the SDK implementation. potential-regression Marking this issue as a potential regression to be checked by team member and removed needs-triage This issue or PR still needs to be triaged. potential-regression Marking this issue as a potential regression to be checked by team member labels Oct 21, 2024
@erezrokah
Copy link
Author

Thanks for the quick response @lucix-aws 🚀 I get it's hard to manage the major bumps with the dependency on the service API changes, and that it might take a while to adhere to semantic versioning.
A good resolution on my end at least would be to communicate the context behind the breaking change and how to handle it

@lucix-aws
Copy link
Contributor

lucix-aws commented Oct 21, 2024

Agreed, that's what we've been doing for recent changes like this and I think it goes a long way in the absence of major versioning bumps. I've asked internally that IF the Athena team intended for this change to happen, that they provide us with more proper release notes that include context/guidance so we can update them in all our SDKs.

@ejeffrli
Copy link

What calls were working before and how are they breaking now?

@erezrokah
Copy link
Author

What calls were working before and how are they breaking now?

It's not about API calls breaking, but changes the models. Data that was there before is now missing

The commit is here. Fields removed are ConnectionType, Error, Status

@lucix-aws
Copy link
Contributor

What calls were working before and how are they breaking now?

It's not that straightforward. A customer calling the SDK can reference a field in any number of ways without doing so in the context of a request/response.

Consider the following:

func CopyCreateDataCatalogOutput(v *athena.CreateDataCatalogOutput) *athena.CreateDataCatalogOutput {
    return &athena.CreateDataCatalogOutput{
        DataCatalog: CopyDataCatalog(v.DataCatalog),
        // ... other fields
    }
}

func CopyDataCatalog(v *types.DataCatalog) *types.DataCatalog {
    return &types.DataCatalog{
        // ...
    }
}

This code, however or for whatever reason it's used, was broken by this change.

The exact purpose of this function or where it's used are irrelevant. We can think of infinite examples across the realm of Turing completeness here. What matters is that the service team has released this field in their expressly-public API, and customers have taken a dependency on it. Essentially, the service team is not the authority on what constitutes possible use of a field, the customer is.

Hashicorp's Terraform is an excellent real-world case study for this kind of thing. Terraform is a multi-cloud IaC tool - think CDK but works with other cloud providers. Terraform uses the Go SDK, and by the nature of the product, it essentially comprehensively references fields in the Go SDK that it needs to support for its users. It may do so without regard to the nature of a particular field, how it's used, or whether it even works - it simply needs to support configuring it and referencing it so the customer using Terraform can use it if need be.

@lucix-aws
Copy link
Contributor

Updated messaging: 3bab200

@lucix-aws lucix-aws self-assigned this Oct 21, 2024
Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

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. potential-regression Marking this issue as a potential regression to be checked by team member 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

3 participants