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

Use MinVer #3562

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Use MinVer #3562

wants to merge 4 commits into from

Conversation

sungam3r
Copy link
Member

@sungam3r sungam3r commented Mar 7, 2023

See https://github.com/adamralph/minver

It allows to not bother with setting version manually in code/csproj. I tested it in 4 repos. So far it works.

@sungam3r sungam3r requested a review from Shane32 March 7, 2023 07:54
@github-actions github-actions bot added the CI CI configuration issue or pull request label Mar 7, 2023
@sungam3r
Copy link
Member Author

sungam3r commented Mar 7, 2023

изображение
😮 Did you notice that?

@@ -1,7 +1,7 @@
<Project>

<PropertyGroup>
<VersionPrefix>7.3.0-preview</VersionPrefix>
<MinVerDefaultPreReleaseIdentifiers>preview</MinVerDefaultPreReleaseIdentifiers>
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -21,6 +21,8 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
with:
fetch-depth: 0
Copy link
Member Author

Choose a reason for hiding this comment

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

MinVer need tags to work. It's a known issue with actions/checkout - this action has no explicit option to fetch tags. fetch-depth: 0 effectivelty fetches all tags. Yep, it fetches all commits as well 😞 .

Copy link
Member Author

Choose a reason for hiding this comment

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

@Shane32
Copy link
Member

Shane32 commented Mar 7, 2023

I don't favor this. First, I don't like having to retrieve the entire commit history, further delaying test runs. But even more importantly, I don't like introducing another build-time dependency when it is not necessary. We already have too many dependencies here that constantly "need" updating. It also makes it more difficult to understand the versioning process by looking at the csproj and workflow files, whereas now it is clear and precise.

What are the benefits? The numbering for dev builds is more "accurate"? Does not matter; they require quite a lot of work to use, and the few users there are (if any) should not care.

So in summary, I see this as something that increases, not decreases, the maintenance and development time required for GraphQL.NET. As an open-source project maintained by only two of us, both of which are extremely pressed for time, I cannot endorse it.

@sungam3r
Copy link
Member Author

sungam3r commented Mar 7, 2023

What are the benefits?

The benefits are to rely entirely on tags when calculating the version and not to change it in the source code and/or scripts. Note that we do not always remember to bump version in Directory.Build.props file so it may be like 7.5.0-preview while 7.5.0 release package was already pushed to nuget.org. Not a big deal though, just small automation.

I'm not going to force this PR. I will let it be here opened for a while and will close it if your opinion does not change. In some of my projects I recently switched to MinVer. I like that everything works simply and without my intervention in versioning.

@Shane32
Copy link
Member

Shane32 commented Mar 7, 2023

Another option is to leave the version in directory.build.props at 20.0.0-preview. It will always generate a newer version than the latest published version. (This only matters if using another published package like the server project in conjunction with a dev build.)

@Shane32
Copy link
Member

Shane32 commented Mar 7, 2023

If I have time perhaps I will test MinVer in one of my own repos

@sungam3r
Copy link
Member Author

sungam3r commented Mar 7, 2023

If I would choose between some kind of fictitious version and the absense of version in sources, then I will prefer the latter.

@sungam3r
Copy link
Member Author

sungam3r commented Mar 7, 2023

If I have time perhaps I will test MinVer in one of my own repos

👍 Honestly it took me a couple of years to finally try, I have long known about this package and saw its use in other famous repositories.

@codecov-commenter
Copy link

Codecov Report

Merging #3562 (f9ecadf) into master (649a8cd) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #3562   +/-   ##
=======================================
  Coverage   83.84%   83.84%           
=======================================
  Files         381      381           
  Lines       16838    16838           
  Branches     2695     2695           
=======================================
  Hits        14118    14118           
  Misses       2074     2074           
  Partials      646      646           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sungam3r
Copy link
Member Author

@Shane32 Did you try? I also worked with gitversion last months. It works well. See https://gitversion.net/docs/.

@Shane32
Copy link
Member

Shane32 commented Oct 11, 2023

I think what we have already works perfectly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI CI configuration issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants