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

chore(deps): Update Rust dependencies #1530

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

reneleonhardt
Copy link
Contributor

@reneleonhardt reneleonhardt commented Jun 16, 2024

Features

  • Update Rust dependencies (tempdir was deprecated)

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@@ -2,7 +2,7 @@ version: 2

updates:
- package-ecosystem: "docker"
directory: "/"
directory: "/.release"
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated? Did you ever wonder why there have been no updates in https://github.com/getsops/sops/commits/main/.release? 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

This has nothing to do with Rust. This PR is for updating Rust dependencies.

serde_json = "1.0.99"
serde_yaml = "0.9.22"
serde_json = "1.0"
serde_yaml = "0.9"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you are removing the patch versions for most of the dependencies? Did you verify that the tests also work fine with lower patch versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems best practice for Rust dependencies to require always the latest patch version in every build.
How does this affect your tests results?
I tested what I could, your project doesn't provide e2e test scripts for developers, so the 2 vault tests failed, therefore contributors have to rely on your CI pipelines.

Wouldn't it be much better for your users if you would care more for build security than allegedly incompatible patch versions of test dependencies? 🤔
https://blog.rust-lang.org/2023/08/03/cve-2023-38497.html
Why are you using an ancient rust 1.70.0 toolchain since 2 days?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems best practice for Rust dependencies to require always the latest patch version in every build.

Sounds good to me.

Wouldn't it be much better for your users if you would care more for build security than allegedly incompatible patch versions of test dependencies? 🤔

I'm not sure what you're trying to imply here. I asked a simple question here, I didn't say you have to revert to the previous state. I personally do care for build security, but I'm not that familiar with all the tools used in the SOPS build pipeline.

https://blog.rust-lang.org/2023/08/03/cve-2023-38497.html Why are you using an ancient rust 1.70.0 toolchain since 2 days?

How come you assume we're using it since 2 days? We have been using Rust 1.70.0 for a long time already, the only thing that changed is the place where 1.70.0 is configured. (Which makes it easier to update that version to something less ancient.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also in CI one often pins all dependencies, and uses things like Dependabot to update them. That prevents random CI failures due to updated dependencies that sneaked in without explicit approval.

Since you added a Dependabot config for Cargo in this PR, I don't really understand why you remove the patch versions here.

@felixfontein
Copy link
Contributor

@reneleonhardt ping

@reneleonhardt reneleonhardt force-pushed the updates-rust branch 2 times, most recently from 54f3d8d to 1fee572 Compare September 14, 2024 15:12
@felixfontein felixfontein added this to the 3.9.1 milestone Sep 14, 2024
@felixfontein
Copy link
Contributor

felixfontein commented Sep 23, 2024

Can you please either remove the Dependabot config for Rust, or re-add the full version numbers in Cargo.toml (the second is preferable from my side)? And remove the Docker dependabot config from this PR? Thanks.

@felixfontein
Copy link
Contributor

Why did you remove the Dependabot config for Rust/Cargo?

@felixfontein
Copy link
Contributor

(Also note that with commit messages like that, the chance that I will merge this are pretty much zero. Maybe someone else will merge it, but I definitely will not.)

@reneleonhardt
Copy link
Contributor Author

You required me to remove the Dependabot config for Rust, even if it makes no sense for an Update Rust dependencies PR, if I want to have a slight chance to get my contribution to your project merged.
So I did against my better judgement, that's why I had to phrase those commits accordingly about the consequences for the maintainers who didn't configure working Dependabot rules yet and made this PR necessary in the first place because no one stepped up and replaced tempdir for example which has been deprecated 6 years ago.
Then you copied my contribution to rip it out-of-context without asking for my permission or if that even would make sense, an Update PR should update and configure Dependabot to update automatically int the future to avoid bugs and security vulnerabilities 🤷

… documentation because they are too short. This means that by disabling Dependabot new updates will not be build and tested automatically, and that the opposite will be true for all other dependencies not updated here (serde and serde_derive are allowed to be short).

Signed-off-by: Rene Leonhardt <[email protected]>
@felixfontein
Copy link
Contributor

You required me to remove the Dependabot config for Rust, even if it makes no sense for an Update Rust dependencies PR, if I want to have a slight chance to get my contribution to your project merged.

I hope you're not actively trying to misinterpret what I wrote:

Can you please either remove the Dependabot config for Rust, or re-add the full version numbers in Cargo.toml (the second is preferable from my side)?

There is an EITHER-OR condition. That means, either do the first, or the second. I also explicitly wrote that I would prefer the second, not the first.

You decided to do both, which is not what I asked you to do, and what we both agree is not a good idea.

Then you copied my contribution to rip it out-of-context without asking for my permission

I did not copy your contribution, I simply recreated parts of it since a) it was no longer part of this PR, and b) it was something I would have liked to merge already in June. Also if you compare the Cargo part of your commit (11ebfc2) with my commit (576f809), you can see that I did not copy your contribution, but based it on the existing entry for GH Actions.

@felixfontein
Copy link
Contributor

felixfontein commented Sep 28, 2024

Also, according to the official Cargo documentation (https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-cratesio), serde_json = "1.0.128" allows all versions of serde_json that are >= 1.0.128 and < 2.0.0, while serde_json = "1.0" allows all versions that are >= 1.0.0 and < 2.0.0. So there's no good reason to shorten version numbers, unless you want to allow even older versions of the dependencies to match.

@felixfontein felixfontein modified the milestones: 3.9.1, Maintenance work Oct 3, 2024
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