Skip to content

Conversation

treagod
Copy link
Contributor

@treagod treagod commented Apr 11, 2025

This PR moves region and endpoint resolution logic from Awscr::S3::Http into Awscr::S3::Client. The HTTP layer now receives a fully resolved URI from the client.

This makes it easier to:

  • Access client.endpoint directly
  • Wrap the client in higher-level tools
  • Reuse the resolved endpoint and region for related functionality (e.g., generating presigned URLs)

The Http class is now purely transport-focused, with no knowledge of regions or endpoint logic.

@miry miry self-assigned this Apr 12, 2025
@miry
Copy link
Collaborator

miry commented Apr 12, 2025

What do you think about doing a soft migration?
For example, we could support both versions for a while—allowing users to upgrade without changing the interface—while showing a warning that the old version is deprecated.

Once the new version is fully released, we can remove the old logic.

@miry miry mentioned this pull request Apr 12, 2025
5 tasks
@treagod
Copy link
Contributor Author

treagod commented Apr 12, 2025

I’m open to it. My initial approach was to expose the endpoint from Http and then surface it through Client. One caveat is that Http gets re-initialized each time the private #http (that's why I'm interested in #77 😁) method is called, though in practice this isn’t a major issue.

Before committing to a soft migration, I’d be interested to know if anyone relies on Http directly, since within the library, only Client seems to use it. If no one else is using Http externally, we can likely proceed without deprecating the old logic. Do you use it?

What do you think?

@miry
Copy link
Collaborator

miry commented Apr 12, 2025

Before committing to a soft migration, I’d be interested to know if anyone relies on Http directly, since within the library, only Client seems to use it. If no one else is using Http externally, we can likely proceed without deprecating the old logic. Do you use it?

I can’t say for sure whether anyone is using Http directly,
but since it’s part of the public interface,
it’s definitely possible that some users are relying on it or even overriding it.
I've seen quite a few libraries and applications that depend on this package,
so we should be cautious.

I’d recommend taking a soft upgrade path—this way, we avoid introducing breaking changes and give users a chance to adapt, ideally with clear upgrade instructions.

UPDATE: If you think it will be difficult to support, let's update CHANGELOG.md to note that this includes breaking changes in HTTP.

@treagod
Copy link
Contributor Author

treagod commented Apr 13, 2025

No I don't think it's too difficult 😊 I thought about it and now the endpoint is stored in both classes for the time being!

That's okay with you?

@treagod
Copy link
Contributor Author

treagod commented Apr 14, 2025

Ping @miry,

can't click re-review for some reason

@miry
Copy link
Collaborator

miry commented Apr 14, 2025

@treagod Can you add your changes to CHANGELOG?
I run your changes in my projects for some time to make sure it works.

@miry miry merged commit 9265cca into taylorfinnell:master Apr 14, 2025
6 checks passed
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.

2 participants