Skip to content

Conversation

@xtylez-eskardinha
Copy link
Contributor

Migrated all the AWS SDK v1 to v2.

May need some checks with a product implementation to confirm it works correctly.

@xtylez-eskardinha
Copy link
Contributor Author

Related to #14

@SuperQ
Copy link
Member

SuperQ commented Apr 14, 2025

Related: #21

@xtylez-eskardinha
Copy link
Contributor Author

Also, checking the docs and for solving #18 we could take a look at: https://pkg.go.dev/github.com/aws/[email protected]/aws#CredentialsCacheOptions

@xtylez-eskardinha
Copy link
Contributor Author

Related to #9

@SuperQ
Copy link
Member

SuperQ commented Apr 23, 2025

Ready for a rebase.

@rapphil
Copy link

rapphil commented Apr 23, 2025

thanks for this PR. I can help by testing it after you address my comment about reusing the buffer.

@xtylez-eskardinha
Copy link
Contributor Author

Changes implemented, however with the new golangci linting I get an error on make test related to formatting:

>> running golangci-lint
/Users/xtylez/go/bin/golangci-lint run  ./...
sigv4_config.go:25:6: exported: type name will be used as sigv4.SigV4Config by other packages, and that stutters; consider calling this Config (revive)
type SigV4Config struct {
     ^
make: *** [common-lint] Error 1

This would break the current plug&play implementations, maybe we should raise exception in the CI for this?

@SuperQ
Copy link
Member

SuperQ commented Apr 24, 2025

You can add a //nolint:revive to that line.

@SuperQ SuperQ requested a review from rapphil April 25, 2025 07:55
@rapphil
Copy link

rapphil commented Apr 25, 2025

Will test the latest revision today and post results here.

@rapphil
Copy link

rapphil commented Apr 26, 2025

While testing the latest version, I'm getting this error in the prometheus logs.

time=2025-04-26T00:02:45.114Z level=WARN source=queue_manager.go:2029 msg="Failed to send batch, retrying" component=remote remote_name=e53241 url=https://aps-workspaces.us-west-2.amazonaws.com/workspaces/ws-xxxxx/api/v1/remote_write err="Post \"https://aps-workspaces.us-west-2.amazonaws.com/workspaces/ws-xxxxxxx/api/v1/remote_write\": net/http: HTTP/1.x transport connection broken: http: ContentLength=11770 with Body length 0"

I'm trying to troubleshoot the issue.

@rapphil
Copy link

rapphil commented Apr 30, 2025

I tested the latest version with the latest version of Prometheus and it works! thanks!

@xtylez-eskardinha
Copy link
Contributor Author

Implemented all the suggested changes, however I've one little concern related to AssumeRole, @rapphil could you test that? I'll ping the review to the exact line

@rapphil
Copy link

rapphil commented Apr 30, 2025

Ok, I tested with/without assume role and it works in both case. I can see the assumed roles credentials being fetched every 15 minutes (a little bit before that actually because of the cache).

@SuperQ
Copy link
Member

SuperQ commented May 5, 2025

This needs a quick rebase to pick up go.mod changes.

@SuperQ
Copy link
Member

SuperQ commented May 5, 2025

Also, if you would please squash the commits into logical changes so the commit log is not so messy.

@xtylez-eskardinha
Copy link
Contributor Author

@SuperQ I think I cleaned a little the commit history, if you need more changes or even smaller commit history let me know!

@rapphil
Copy link

rapphil commented May 20, 2025

@SuperQ can comment before you take any action, but IMHO this PR is small enough to be a single commit. So if SuperQ agrees, maybe you can squash everything into a single commit.

@xtylez-eskardinha
Copy link
Contributor Author

I can squash everything to one commit if you feel like it, however, I don't see the difference between doing that in my git branch and setting that on the PR config, as this repo doesn't have high PR/Commit rate

@SuperQ
Copy link
Member

SuperQ commented May 20, 2025

I can squash and merge, but then I have to manually cleanup the commit message. Please squash your commit history so this is only one commit.

In the future, I recommend you use "fork and branch" PR workflows so you do not have changes to main in your PRs.

@xtylez-eskardinha
Copy link
Contributor Author

Done @SuperQ , thank you for your advice about fork and branch, I'll for next PRs :)

@SuperQ
Copy link
Member

SuperQ commented May 21, 2025

This still contains a ton of unrelated commits.

image

Signed-off-by: Miguel Angel Sanchez <[email protected]>
Signed-off-by: Miguel Angel Sanchez <[email protected]>
Signed-off-by: Miguel Angel Sanchez <[email protected]>
@xtylez-eskardinha
Copy link
Contributor Author

Will reopen new PR correctly, I wasn't able to recover git history

@xtylez-eskardinha
Copy link
Contributor Author

@SuperQ @rapphil #31 new PR with correct commit history

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.

3 participants