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

Combine dependabot security updates #1831

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

jmwample
Copy link
Contributor

@jmwample jmwample commented Dec 20, 2024

@pronebird
Copy link
Contributor

Some of these are major updates. We might need to verify if things are still working as expected.

Doing this in bulk across many apps could be problematic especially with a lot of people on holidays at the moment.

@jmwample
Copy link
Contributor Author

jmwample commented Dec 20, 2024

This is definitely true, I was playing with combining all of them to knock them out since many of them are not breaking changes and close significant bugs. However, I have discovered that dependabot suggesting to just do things like migrate to react v19 are much bigger changes. So I will likely leave that specifically out.

edit: or try a less agressive bump i.e. to ^18.2.0 -> ^18.3.0

@zaneschepke
Copy link
Contributor

I'll need to test this on Android because a few of these packages broke android in the past when we upgraded

@pronebird
Copy link
Contributor

pronebird commented Dec 20, 2024

This one is no longer applicable

#1601

This one was approved so I simply merged it

#1409

@jmwample
Copy link
Contributor Author

I'll need to test this on Android

Noted. I am not trying to merge this immediately and shake things up on Friday evening before holidays, just do some house cleaning while I have the time 😄

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Looks sane 👍

@pronebird pronebird self-requested a review December 21, 2024 10:32
@@ -39,6 +39,11 @@ vergen = { workspace = true, default-features = false, features = [
"cargo",
] }

[target.'cfg(windows)'.dependencies]
Copy link
Contributor

Choose a reason for hiding this comment

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

Neither of these crates seem to be used in nym-vpnc based on a quick search with Github

@@ -64,7 +64,7 @@ nym-vpnd-types = { path = "../nym-vpnd-types" }
[target.'cfg(windows)'.dependencies]
windows-service = "0.7.0"
eventlog = "0.3.0"
winapi = { version = "0.3", features = ["winnt", "excpt"] }
winapi = { version = "0.3", features = ["winnt", "excpt", "winerror"] }
Copy link
Contributor

@pronebird pronebird Dec 21, 2024

Choose a reason for hiding this comment

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

FYI Not a problem with this PR but to raise awareness. We've been putting effort at consolidating dependencies in the workspace Cargo.toml so it's easier to keep track of them. IMO features can be specified by each indiviudal crate, that's ok, the effort is to move versions into workspace toml.

@jmwample jmwample force-pushed the dependabot/12-24_combined branch from e1dadf5 to af0b57f Compare December 24, 2024 18:41
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