Skip to content

Conversation

@Shane32
Copy link
Member

@Shane32 Shane32 commented Sep 16, 2020

This PR adds GitHub Actions (workflows) for the following purposes:

  • Test PRs on windows and ubuntu with .NET 2.2 and .NET 3.1
  • Create nuget packages when a PR is merged (or any other commit to master or develop) and publish them to the GitHub Packages repository. Versions have the prefix specified in the build props file, with a suffix that increments starting a little above 1000.
  • Publish nuget packages to Nuget when a release is issued, using the exact version number specified in the "tag" of the release.

This does not publish to myget. This could be enabled by changing the build.yml file a little.

In order to have this code publish to Nuget, the API key for Nuget must be loaded into a NUGET_AUTH_TOKEN GitHub secret. See Settings > Secrets. Secrets cannot be read by workflows in forked repositories, so they are secure. Who holds the proper Nuget account? They will need to generate a new API key and punch it in.

I've tested as much as I can at this point. I'd prefer to merge, issue a release, and review the artifacts generated. It will not publish to nuget because it will not have an API key. If all is well, then add the NUGET_AUTH_TOKEN and delete and reissue the release.

@Shane32 Shane32 self-assigned this Sep 16, 2020
@Shane32 Shane32 requested review from pekkah and sungam3r September 16, 2020 19:31
@Shane32 Shane32 marked this pull request as ready for review September 16, 2020 19:31
@Shane32
Copy link
Member Author

Shane32 commented Sep 17, 2020

Sourcelink should be working now. Based on:

https://devblogs.microsoft.com/nuget/introducing-source-code-link-for-nuget-packages/

Also tested generated packages with Nuget Package Explorer

@sungam3r
Copy link
Member

Thank. I'll put this PR aside for now to finish other things. Now there is no urgent need to build the release. In parallel, I will read the documentation on github actions and appveyor. As a result, I would like to choose the simplest and most understandable solution with a minimum of scripts.

@sungam3r sungam3r added the CI CI configuration issue or pull request label Sep 17, 2020
@Shane32
Copy link
Member Author

Shane32 commented Sep 17, 2020

with a minimum of scripts

I agree.

We can delete these files if we remove appveyor:

  • appveyor.yml
  • build.cake
  • build.ps1
  • build.sh
  • gitversion.yml
  • tools/packages.config

most understandable

I agree.

GitHub Actions is only a single script for a single action. We have three actions, so there are three independent scripts.
Personally I think this is part of what makes GitHub Action scripts easier to understand.

@sungam3r
Copy link
Member

@Shane32 I propose to change version template for GraphQL.NET preview packages. Now it is x.y.z.b where b is a github build number. It is not semver compatible version. Let's change it to x.y.z-preview.b like MS does.

@Shane32
Copy link
Member Author

Shane32 commented Sep 17, 2020

Done. Start at 1 or 1001?

@sungam3r
Copy link
Member

I prefer 1. But I talked about GraphQL.NET, not server project. By the way, how to reset github build number?

@Shane32
Copy link
Member Author

Shane32 commented Sep 17, 2020

Would we not want a consistent build process for all the projects?

We can reset the number by renaming the yml file, as noted in the comment in the yml file.

@sungam3r
Copy link
Member

Would we not want a consistent build process for all the projects?

Sure. I just drew attention to the fact that it is worth changing the counter in the already committed code in graphql-net.

.AddWebSockets();
services.AddLogging(builder =>
{
// prevent writing errors to Console.Error during tests (required for testing on ubuntu)
Copy link
Member

Choose a reason for hiding this comment

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

Wow! Does this cause the tests to fail?

Copy link
Member

Choose a reason for hiding this comment

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

What about allow to write logs on Windows?

if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
  // Do something
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I can’t remember anymore. Maybe it just caused a bunch of extra output. I think it caused a failure though, without this change. I did write “required”...

Copy link
Member

Choose a reason for hiding this comment

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

Well, OK.

Shane32 and others added 3 commits October 26, 2020 10:06
@sungam3r
Copy link
Member

Thanks for this PR. One question left : VersionSuffix. I still don't understand why this is needed.
I'm OK with Deterministic=true event if it is true by default.

@Shane32
Copy link
Member Author

Shane32 commented Oct 26, 2020

It's not needed, per se. It just ensures that any local build will not have a "real" version number. Actually, nevermind. Since the default prefix has "preview" in it already, that should be covered. I'll remove that.

Co-authored-by: Ivan Maximov <[email protected]>
Copy link
Member

@sungam3r sungam3r left a comment

Choose a reason for hiding this comment

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

The last suggestion is to move <VersionPrefix>4.1.0-preview</VersionPrefix> on the first line inside props section.

@Shane32 Shane32 merged commit 31d3858 into develop Oct 26, 2020
@Shane32 Shane32 deleted the github-packages branch October 26, 2020 16:57
@sungam3r
Copy link
Member

By the way, here I configured branches https://github.com/graphql-dotnet/server/settings/branches for both master and develop but in GraphQL.NET I can see only master rule https://github.com/graphql-dotnet/graphql-dotnet/settings/branches.

@Shane32 Shane32 mentioned this pull request Oct 26, 2020
Shane32 added a commit that referenced this pull request Oct 26, 2020
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 performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants