-
Notifications
You must be signed in to change notification settings - Fork 5
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
Checks in PRs from Dependabot will always fail #64
Comments
How about we replace the versions in the PACKAGES.md file with links to the relevant csproj? Also these versions are really minimum versions anyway, since the actual version restored my be different than specified. |
Not sure I understand this statement. I thought The only time you would get a later version is if a downstream project references the csproj and also a higher version of that dependency. Normally that only happens when the build output is a nuget package. But I do feel that the Packages.md is pretty redundant now that there is: https://github.com/SMI/SmiServices/network/dependencies @Tallmaris what do you think about scrapping the Packages markdown and/or adding a link to the dependencies network page (since this was initially added as an audit thing across all projects as part of ISO certification) |
TL;DR: https://github.com/Yelp/requirements-tools#our-stance-on-pinning-requirements (although python specific) For direct dependencies, yes. For deps of deps / transitive deps which aren't pinned, the versions that are actually restored can vary depending on the particular resolution strategy and the time at which the resolution is performed. Although we should always end up with some valid combination of packages, there can still be variance. If e.g. You have a dep chain of A -> B -> C, even if you pin B to a specific version in A, version C can still float if newer versions of it are released and nuget just grabs the newest version at build time. There is a distinction depending on whether you're talking about an end-application and a library though. In a library you want to be really permissive so that your downstream users can be free to choose specific requirements, and you should therefore only specify minimum versions of your dependencies. That way if newer versions of a dependency break you library you'll find out pretty quickly. For applications, you want to enable completely repeatable builds, so using a lockfile will pin every dependency to a specific version. So... |
So for us, I agree that providing the package.md files are useful for a high-level view of what we depend on, but specifying versions and testing this file doesn't really seem practical since its something else to maintain (and doesn't play nicely with dependabot as we've seen here). For lockfile support, I'm not sure what the best option is. The support for it natively in the dotnet cli was pretty new last time I looked and I remember it not working very well, although I admit I don't remember the exact issue. The other option is to use an external package manager such as Paket which does do this sort of thing very well, but which Dependabot doesn't currently support (although this is being worked on by me dependabot/dependabot-core#1944 !). The other nice thing that Paket has, which I don't think dotnet does, is an easy way to manage all dependencies for a solution in one place rather than in each individual csproj file. I believe this is an important factor since we probably don't want dependencies to have different versions for projects within a single solution and, if so, does that suggest they really shouldn't be in the same solution after all? I remember one case we had recently in this repo where fo-dicom was unintentionally upgraded in one of the sub-projects and made it to the production system without us knowing. Since we (still) package all the outputs together, and don't have a lockfile, that newer version overwrote the previous one and all the projects were suddenly using the new version! Having multiple lockfiles for each sub-project also doesn't feel easy to maintain imo. Relevant NuGet issues: |
My take - specifying version numbers in Packages.md is generally redundant, particularly when as @tznind points out we already have an automatically-updated page which tracks exactly that (for the release branch, at least). For now, we could just drop the version numbers from Packages (and the checks on them) but keep the list of packages, maybe adding a link to where the latest dependency list can be found. (Perhaps also include a snapshot of that page as part of the release process in future?) That eliminates the redundant information and means Dependabot PRs won't fail on that hurdle, without too much extra effort or change. Longer term, consider switching from csproj to Paket (or similar): it sounds like it would be an improvement overall, but not an option just yet? |
Ok I've put in the PR to drop version since that is what the title and original issue was about. We can create a new issue for Packet/dependency lock if you want as an enhancement or leave it on the back burner. |
Thanks Thomas. I'd like to take a look at how paket works in more detail before we make a decision on switching, e.g. determine how well it integrates with Visual Studio. |
The tests which validate the Packages.md contents will (obviously) fail as they won't be updated to match the version bump that Dependabot is suggesting, so every Travis build will fail. This is pretty minor, so just opening this for awareness in case we have time soon to think of a fix (hah!).
The text was updated successfully, but these errors were encountered: