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

Cutting out a new release #711

Closed
stormshield-gt opened this issue Jul 29, 2024 · 10 comments · Fixed by #713
Closed

Cutting out a new release #711

stormshield-gt opened this issue Jul 29, 2024 · 10 comments · Fixed by #713
Milestone

Comments

@stormshield-gt
Copy link

stormshield-gt commented Jul 29, 2024

I will be very interested if you could please cut out a new minor release patch release.
I'm looking into the upgrade of rustls to 0.23 which we just get through #710 that update bollard to 0.17

@DDtKey
Copy link
Collaborator

DDtKey commented Jul 29, 2024

Hi @stormshield-gt 👋

Do you want us to rollout a new minor release with #710 included?
I wanted to prepare a major release soon, there are some breaking changes in main.

Does it work for you? Otherwise we might consider a patch release with only this change, however it's not a fix

@stormshield-gt
Copy link
Author

I wanted to prepare a major release soon, there are some breaking changes in main.

Did you mean minor release ? Or you are going 1.0.0 ? It any case it will definitely work for me.

Otherwise we might consider a patch release with only this change, however it's not a fix

Because this crate has not reached 1.0.0 yet, I think it's OK to use patch release for all non-breaking changes. If we use minor release even for non-breaking changes, it adds a burden to downstream users.
For instance, 0.20 is not semVer compatible with 0.21 so it won't be updated automatically by cargo update, and users will have to manually update their Cargo.toml files.
rustls for instance use patch release for non-breaking changes.

@stormshield-gt stormshield-gt changed the title Cutting out a new minor release Cutting out a new release Jul 29, 2024
@DDtKey
Copy link
Collaborator

DDtKey commented Jul 29, 2024

By major I mostly was referring to "breaking one", meaning minor part of semver.
Just wanted to highlight that there are breaking changes and we use minor part for these.

It will be 0.21.0

For instance, 0.20 is not semVer compatible with 0.21 so it won't be updated automatically by cargo update, and users will have to manually update their Cargo.toml files.

That's definitely clear 🙂 I just wanted to understand if you want compatible change or not - because this would require backport into 0.20.x

@stormshield-gt
Copy link
Author

By major I mostly was referring to "breaking one", meaning minor part of semver.

How I see what you meant.

I just wanted to understand if you want compatible change or not - because this would require backport into 0.20.x

I have time to update to 0.21.0 breaking changes if they are, so for me it will be OK to not do the backport. Whatever it’s the simplest for you.

@DDtKey
Copy link
Collaborator

DDtKey commented Jul 29, 2024

Great, thanks for confirming
I'll do this within the day then, as fallback option I'll backport it

@DDtKey
Copy link
Collaborator

DDtKey commented Jul 29, 2024

Actually, we can't backport this one
In general, upgrade of rustls to 0.23 is breaking change and may affect users - there are some platform-related limitations in this version (e.g build in Windows is highly affected). Since the features are additive, we can't guarantee it won't break someone's build

Btw, what's the issue with having 0.22 & 0.23? They can co-exist.
And testcontainers is usually only dev-dependency. What's the reason of a rush?

@stormshield-gt
Copy link
Author

Actually, it seems that bollard wants to force the ring provider that doesn't have the NASM requirement on windows.

But I think they didn't disable default features so maybe they are bringing both ring and aws-lc-rs provider, I will check that and open an issue there. I think they should re-export the provider as features, as did the rest of the ecosystem like tokio-rustls or hyper-rustls.

Besides, as docker is more linux/unix oriented, maybe Windows is not such a concern. The workaround is just to add a dependency on NASM if someone really wants to run test container on windows.

Btw, what's the issue with having 0.22 & 0.23? They can co-exist.
And testcontainers is usually only dev-dependency. What's the reason of a rush?

No need for rush, I opened the issue to say that they are a need, but I don't want to put any pressure. Sorry if I did.
For me it's just a matter of denying rustls 0.22 everywhere, so my coworkers are forced to use the process default provider, even for not dev dependency. Maybe tools like cargo deny can allow a crate only for dev-dependency

@DDtKey
Copy link
Collaborator

DDtKey commented Jul 29, 2024

Actually, it seems that bollard wants to force the ring provider that doesn't have the NASM requirement on windows.

But I think they didn't disable default features so maybe they are bringing both ring and aws-lc-rs provider, I will check that and open an issue there. I think they should re-export the provider as features, as did the rest of the ecosystem like tokio-rustls or hyper-rustls

Yeah, they don’t disable default ones. And anyway - features are additive. Some another dep may enable it. So it’s kinda fragile

Besides, as docker is more linux/unix oriented, maybe Windows is not such a concern. The workaround is just to add a dependency on NASM if someone really wants to run test container on windows.

I think there is no problem in it. I’m only wanted to highlight that it can’t be considered as “compatible” change unfortunately

No need for rush, I opened the issue to say that they are a need, but I don't want to put any pressure. Sorry if I did.
For me it's just a matter of denying rustls 0.22 everywhere, so my coworkers are forced to use the process default provider, even for not dev dependency. Maybe tools like cargo deny can allow a crate only for dev-dependency

That totally makes sense, just was wondering if you have some special need and we’re blocking you 🙂
Release is going to be soon anyway

@DDtKey DDtKey added this to the 0.21.0 milestone Jul 30, 2024
@DDtKey DDtKey closed this as completed in f8d4a3d Jul 30, 2024
@DDtKey
Copy link
Collaborator

DDtKey commented Jul 30, 2024

Hi @stormshield-gt 👋

I've released 0.21.0 version

@stormshield-gt
Copy link
Author

Hi @DDtKey , thanks a lot! I've done a PR to bollars to solve the problem we talk about by the way fussybeaver/bollard#441

DDtKey pushed a commit that referenced this issue Aug 3, 2024
Thanks again for working on this project!

This exposes a way to configure resource ulimits (the
[`--ulimit`](https://docs.docker.com/reference/cli/docker/container/run/#ulimit)
flag for `docker run`).

I'm relying on CI tests because I'm currently on Windows and didn't
configure my environment to reflect
#711, since it
seems I'd need to install nasm now, as I received some errors regarding
it.
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 a pull request may close this issue.

2 participants