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

Manage AWS SDK(s) version with Dependabot #39738

Merged

Conversation

zmoog
Copy link
Contributor

@zmoog zmoog commented May 27, 2024

Proposed commit message

Set up Dependabot to manage the AWS SDK version.

With the current reactive and manual process, our dependencies are often outdated. To release a bugfix to a dependency, we need to wait for the following stack release instead of merging it shortly after it's available from AWS.

See #39492 to learn more.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label May 27, 2024
@zmoog zmoog changed the title Add github.com/aws/aws-sdk-go-v2/* Manage AWS SDK(s) version with Dependabot May 27, 2024
@mergify mergify bot assigned zmoog May 27, 2024
Copy link
Contributor

mergify bot commented May 27, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @zmoog? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@zmoog
Copy link
Contributor Author

zmoog commented May 27, 2024

The following dependencies are AWS-related, but I'm not sure they depend on the AWS SDK.

github.com/awslabs/kinesis-aggregation/go/v2
github.com/aws/smithy-go
github.com/awslabs/goformation/v4
github.com/aws/aws-lambda-go

@zmoog zmoog added the Team:obs-ds-hosted-services Label for the Observability Hosted Services team label May 27, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 27, 2024
@zmoog zmoog marked this pull request as ready for review May 27, 2024 04:32
@zmoog zmoog requested a review from a team as a code owner May 27, 2024 04:32
@elasticmachine
Copy link
Collaborator

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

@zmoog
Copy link
Contributor Author

zmoog commented May 27, 2024

The following dependencies are AWS-related, but I'm not sure they depend on the AWS SDK.

github.com/awslabs/kinesis-aggregation/go/v2
github.com/aws/smithy-go
github.com/awslabs/goformation/v4
github.com/aws/aws-lambda-go

github.com/awslabs/goformation/v4, github.com/aws/smithy-go, and github.com/aws/aws-lambda-go do not directly depend on the AWS SDK.

github.com/awslabs/kinesis-aggregation depends on github.com/aws/aws-sdk-go-v2/service/kinesis

@zmoog
Copy link
Contributor Author

zmoog commented May 27, 2024

github.com/awslabs/kinesis-aggregation depends on github.com/aws/aws-sdk-go-v2/service/kinesis

It seems Functionbeat only uses this package to reference the types.Record struct.

@kaiyan-sheng
Copy link
Contributor

It seems Functionbeat only uses this package to reference the types.Record struct.

Functionbeat is also using the deaggregator: https://github.com/elastic/beats/blob/main/x-pack/functionbeat/provider/aws/aws/transformer/transformer.go#L18

@zmoog
Copy link
Contributor Author

zmoog commented Jun 4, 2024

It seems Functionbeat only uses this package to reference the types.Record struct.

Functionbeat is also using the deaggregator: https://github.com/elastic/beats/blob/main/x-pack/functionbeat/provider/aws/aws/transformer/transformer.go#L18

Oh, good catch!

It seems the awslabs/kinesis-aggregation/blob/master/go/deaggregator/deaggregator.go only use the github.com/aws/aws-sdk-go-v2/service/kinesis package to reference the types.Record struct.

We can probably initially focus on the AWS SDK only (github.com/aws/aws-sdk-go-v2/*), leaving other AWS-related packages for a later addition.

@zmoog
Copy link
Contributor Author

zmoog commented Jul 5, 2024

We need to revise this PR after merging #40125

@zmoog zmoog force-pushed the zmoog/manage-aws-sdk-dependencies-with-dependabot branch from 904d116 to 2c0520b Compare July 8, 2024 15:33
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM (CI is green)

@zmoog zmoog marked this pull request as draft July 22, 2024 13:08
@zmoog
Copy link
Contributor Author

zmoog commented Jul 22, 2024

I am waiting for #40150 to merge before rebasing on it.

@zmoog zmoog force-pushed the zmoog/manage-aws-sdk-dependencies-with-dependabot branch from 2c0520b to 216a477 Compare July 25, 2024 13:22
@zmoog
Copy link
Contributor Author

zmoog commented Jul 25, 2024

I rebased on main to get the changes we made for Azure and GCP.

@rowlandgeoff, would you mind taking a final look at the AWS config?

@zmoog zmoog marked this pull request as ready for review July 25, 2024 13:31
@pazone
Copy link
Contributor

pazone commented Jul 25, 2024

Not sure if it's important, but we also use aws-lambda-go here

@@ -45,6 +47,9 @@ updates:
- dependency-name: cloud.google.com/go/*
groups:
# Cloud providers' SDK dependencies
aws-sdks:
patterns:
- "github.com/aws/aws-sdk-go-v2/*"
Copy link
Member

Choose a reason for hiding this comment

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

should we add github.com/aws/aws-lambda-go, github.com/aws/smithy-go and similar libs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it makes sense! I created separate PRs to facilitate this kind of conversation.

Do we want to group these libraries in the AWS SDK (also adding to groups.aws-sdks.patterns), or is it better to keep them separate (by only including them in the allows list)?

We grouped all the AWS SDK in a group with the github.com/aws/aws-sdk-go-v2/* pattern to avoid dependencies problems like #39454 (a PR accidentally upgraded only the core library, breaking main).

Copy link
Contributor Author

@zmoog zmoog Jul 26, 2024

Choose a reason for hiding this comment

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

I suggest adding github.com/aws/aws-lambda-go and github.com/aws/smithy-go as dependencies to check if we are unsure. We can still add them later to the group.

@rowlandgeoff
Copy link
Contributor

I rebased on main to get the changes we made for Azure and GCP.

@rowlandgeoff, would you mind taking a final look at the AWS config?

Happy to help if you need to be unblocked, but will defer to engineers closer to the code. Based on the comments, looks like sign-off will wait for now.

@zmoog
Copy link
Contributor Author

zmoog commented Jul 26, 2024

Happy to help if you need to be unblocked, but will defer to engineers closer to the code. Based on the comments, looks like sign-off will wait for now.

Yeah, @rowlandgeoff sorry for the ping. The conversation got a new (and welcome) life right after I mentioned you. I'll request a new review when we complete the changes.

Adding the following dependencies:

- github.com/aws/aws-lambda-go
- github.com/aws/smithy-go
@zmoog zmoog requested a review from kruskall July 26, 2024 14:12
@zmoog
Copy link
Contributor Author

zmoog commented Aug 1, 2024

@rowlandgeoff, I guess we're now ready for your review. We can make additional changes in future PRs as needed.

@rowlandgeoff
Copy link
Contributor

LGTM, but I will defer to @dliappis since he's back from PTO

@zmoog
Copy link
Contributor Author

zmoog commented Aug 5, 2024

Thanks! @dliappis, waiting for your feedback before merging the PR, then! 😇

@dliappis
Copy link
Contributor

dliappis commented Aug 5, 2024

LGTM still from an eng prod PoV

@zmoog zmoog merged commit d9e6017 into elastic:main Aug 5, 2024
9 checks passed
@zmoog zmoog deleted the zmoog/manage-aws-sdk-dependencies-with-dependabot branch August 5, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:obs-ds-hosted-services Label for the Observability Hosted Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants