search

rr-wfm / MSBuild.Sdk.SqlProj

An MSBuild SDK that provides similar functionality to SQL Server Data Tools (.sqlproj) projects
MIT License
379 stars 42 forks source link

pin rules versions #543

Closed ErikEJ closed 3 months ago

ErikEJ commented 3 months ago

fixes #524 PR comments

jmezach commented 3 months ago

This does have the side-effect that doing a build today might produce a different result than doing a build tomorrow. This makes it a bit harder to call out any changes to the includes rules in our release notes. Or do we not care about that?

ErikEJ commented 3 months ago

@jmezach How about not doing this, and consider the rules a dependency like DacFX? (so keep as is) ?

ErikEJ commented 3 months ago

I think it makes sense to care about the rule updates, as the will appear as an integral part of the tool going forward

jeffrosenberg commented 3 months ago

I think it makes sense to care about the rule updates, as the will appear as an integral part of the tool going forward It does make sense, but the challenge is that frequent changes bumping the version number of the dependency don't actually give us anything to review, so it's just a few PRs a week to rubber stamp.

@ErikEJ do these repos serve any purpose outside of this project? Would it possibly make sense to bring them into this repo so that we could review the actual changes themselves?

If you don't want to that -- and there are definitely valid reasons not to -- is there a way to bring these into dependabot or otherwise allow merging to trigger a build without requiring 2 PR reviews?

ErikEJ commented 3 months ago

do these repos serve any purpose outside of this project?

Yes, they do, see my blog post https://erikej.github.io/dacfx/codeanalysis/sqlserver/2024/04/02/dacfx-codeanalysis.html (and I also plan some VS tools based on the packages)

is there a way to bring these into dependabot or otherwise allow merging to trigger a build without requiring 2 PR reviews?

Yeah, it is my understanding that this will happen today, it is just me being pushy 😄

jmezach commented 3 months ago

I think these packages should show up in dependabot, at least when there is a stable version, without us having to do anything. I usually merge those PR's without a second reviewer anyway. Might have to look into seeing if that could be automated for dependabot PR's somehow, but that would be a nice to have imho.

jeffrosenberg commented 3 months ago

Okay, now that there's a stable version and we can track in Dependabot, I think that satisfies my concern and we can cancel this change 🙂