Skip to content

Allow excon 1.x and require base64 for modern rubies#8

Merged
nu12 merged 5 commits intonu12:mainfrom
zarqman:excon-1-x
Nov 6, 2024
Merged

Allow excon 1.x and require base64 for modern rubies#8
nu12 merged 5 commits intonu12:mainfrom
zarqman:excon-1-x

Conversation

@zarqman
Copy link
Copy Markdown
Contributor

@zarqman zarqman commented Oct 31, 2024

This PR updates the gemspec to:

  • Allow excon 1.x
  • Update excon minimum to 0.97, which appears to have already been required anyway
  • Require base64 to silence the deprecation warning on Ruby 3.3

Local tests have been flaky and the swarm tests don't seem to run with my dockerd at all. However, apart from the swarm tests, I was able to get everything else to pass once. That might require updates to some of the GitHub URLs. I've excluded those updates from this PR as I'm not sure if they are really required or not. I'm happy to add them to the PR if that'd be helpful.

@nu12
Copy link
Copy Markdown
Owner

nu12 commented Nov 1, 2024

Hi @zarqman , thank you for this PR, your contribution is appreciated.

I believe some responses changed from 1.43 to 1.46, that would require some refactoring of the tests so that they pass. We can either add it to this PR or merge another branch before merging this one. In any case, we might also bump bundler version as the pipeline is not working at all.

Any thoughts on bumping min ruby version to 3.0 instead of 2.3 ?

@zarqman
Copy link
Copy Markdown
Contributor Author

zarqman commented Nov 1, 2024

@nu12, setting a minimum ruby of 3.0 seems fine to me. Updating bundler also makes sense. I don't have a preference for adding it to this PR vs a separate one.

We do have some systems on dockerd from September 2023, and those only support API 1.43. My understanding is that any given dockerd version supports a range of older API versions, so it shouldn't be necessary to force the latest. That said, do we perhaps need to tell dockerd we're using something other than the latest version?

@nu12
Copy link
Copy Markdown
Owner

nu12 commented Nov 2, 2024

Let's group all versioning modification in this PR. If you can manage to fix the tests too, it will be welcomed.
I acknowledge that tests have always been inconsistent. Maybe this is something to be improved later (in the form of simplification).

For the API version, this is what we have in the documentation:

A given version of the Docker Engine SDK supports a specific version of the Docker Engine API, as well as all earlier versions.

The Docker API is backward-compatible, so you don't need to update code that uses the API unless you need to take advantage of new features.

And, more important:

When using curl directly, specify the version as the first part of the URL. For instance, if the endpoint is /containers/ you can use /v1.46/containers/

Source: https://docs.docker.com/reference/api/engine/

But I'd like to implement this part in a separate PR.

@nu12
Copy link
Copy Markdown
Owner

nu12 commented Nov 5, 2024

Hi @zarqman

Support to older API versions is implemented with #9.

I had to make a few adjustments to pass the tests, everything should be good to go now.

You can rebase your branch to integrate these changes. Is it possible to upgrade minimum excon version to 1.x instead of 0.97?

I'll wait for this PR to release a new version. Thank you.

@zarqman
Copy link
Copy Markdown
Contributor Author

zarqman commented Nov 5, 2024

Rebased and Ruby bumped to 3.0. I took the liberty to cleanup some Ruby warnings too.

Between #9 and a couple other minor changes, tests run and pass on both excon 0.97 and 1.1.1.

Since excon was at 0.x for so long, and some other gems still have it locked to 0.x, I left the excon minimum at 0.97. dockerapi's usage of excon is pretty basic and both 0.x and 1.x work fine for now.

Let me know if anything else needs to be changed.

@nu12
Copy link
Copy Markdown
Owner

nu12 commented Nov 5, 2024

Just one thing, encapsulating print_response_to_stdout gives an error if we call the getter without initializing the variable:
image

@zarqman
Copy link
Copy Markdown
Contributor Author

zarqman commented Nov 6, 2024

Good catch. Fixed!

Copy link
Copy Markdown
Owner

@nu12 nu12 left a comment

Choose a reason for hiding this comment

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

LGTM

@nu12
Copy link
Copy Markdown
Owner

nu12 commented Nov 6, 2024

Thank you @zarqman

@nu12 nu12 merged commit 3fa7360 into nu12:main Nov 6, 2024
@zarqman zarqman deleted the excon-1-x branch April 1, 2025 20: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.

2 participants