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

workaround broken dependabot #2455

Merged
merged 2 commits into from
Feb 28, 2024
Merged

Conversation

danmoseley
Copy link
Member

@danmoseley danmoseley commented Feb 27, 2024

This should work around dependabot/dependabot-core#8490 but it will create 232 warnings like

C:\git\aspire\Directory.Packages.props(3,3): warning MSB4011: "C:\git\aspire\eng\Versions.props" cannot be imported aga
in. It was already imported at "C:\Users\danmose\.nuget\packages\microsoft.dotnet.arcade.sdk\8.0.0-beta.24113.2\tools\D
efaultVersions.props (15,3)". This is most likely a build authoring error. This subsequent import will be ignored. [C:\
git\aspire\playground\ProxylessEndToEnd\ProxylessEndToEnd.ApiService\ProxylessEndToEnd.ApiService.csproj]

@joperezr do you know a way to disable these warnings centrally these days? If not, we can put this into another branch, and periodically set that to the default branch temporarily (so it's dependabot's target) and manually run dependabot, then merge any dependabot changes to main.. or something ...

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Feb 27, 2024
@danmoseley danmoseley added area-engineering-systems and removed area-integrations Issues pertaining to Aspire Integrations packages labels Feb 27, 2024
@danmoseley
Copy link
Member Author

aha -- a condition should do it!

@@ -1,4 +1,6 @@
<Project>
<!-- Workaround https://github.com/dependabot/dependabot-core/issues/8490 -->
<Import Project="eng/Versions.props" Condition="'$(MajorVersion)' == ''"/>
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, typically when doing this a new property would get added to Versions.props called something like VersionPropsAlreadyImported or something like that, and then that is what is checked here. Mainly in case MajorVersion gets defaulted somewhere else or moved which might cause issues in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's temporary and if that happens there'll be 200 warnings so we'll know!

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

I'm fine with this as a workaround, but would you mind explaining a bit more about the original issue and why this fixes it if possible?

Also, were you able to do a "dry run" or something similar (not familiar with dependabot so not sure if its even possible) to ensure that this change does workaround the issue?

@danmoseley
Copy link
Member Author

Good point, let me do this in my fork to check it does actually work.

@danmoseley
Copy link
Member Author

danmoseley commented Feb 27, 2024

OK testing this in my fork, it gets further, and makes PR's. They're only in global.json (maybe there's nothign to update currently in the directory.packages.props?). @joperezr can you look at these PR's and check we want PR's like this? https://github.com/danmoseley/aspire/pulls

after that, dependabot still ends up failing, with errors like this.

updater | Updating dotnet-tools.json files.
updater |     Dependency [OpenTelemetry.Instrumentation.AspNetCore] not found in any dotnet-tools.json files.
updater | Running for project file [playground/orleans/OrleansAppHost/OrleansAppHost.csproj]

with various package names.

Nevertheless, I suspect it's now "useful" to merge this.

@danmoseley
Copy link
Member Author

scratch that, this is the real issue "Property 'NetCurrent' was not found.". Let me slap in another workaround and see.

@danmoseley danmoseley marked this pull request as draft February 27, 2024 18:50
@danmoseley
Copy link
Member Author

If I also add this other hack
de5eb21
then it gets further -- but still does not produce any PR's into directory.packages.props, despite changing version numbers back.

@joperezr is there value in the global.json PR's that it did create in my fork? If so, I suggest we merge this as is. It's low risk.

If not, I suggest I close this and we wait for their workaround dependabot/dependabot-core#8490 (comment)

@joperezr
Copy link
Member

@joperezr is there value in the global.json PR's that it did create in my fork? If so, I suggest we merge this as is. It's low risk.

bummer that the workaround isn't yet making the main dependencies we care about work. Regarding the global.json PRs that it does fix, IMO those are a bit less useful in the sense that these are tools that we use during our build, but not dependencies that get eventually deployed with user applications (as their version is only relevant when we build our repo). Those aren't really the primary reason why we'd want to setup dependabot. So my conclusion is that they would have some value, but just not the real value we are trying to get out from dependabot.

Those tools still bugfix things, so I wouldn't discard the workaround in benefit of those PRs, my only concern would be about the potential of new tools affecting our build assets and not being able to catch those breaks during our build.

@danmoseley
Copy link
Member Author

OK I think there is some value and minimal risk in merging this for now just to get those PR's. Or happy to close. LMK

@danmoseley danmoseley marked this pull request as ready for review February 28, 2024 05:58
@danmoseley danmoseley merged commit fff8bea into dotnet:main Feb 28, 2024
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants