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

Add support for TagsMetadata in endpoints #2210

Merged

Conversation

captainsafia
Copy link
Contributor

@captainsafia
Copy link
Contributor Author

@domaindrivendev So, this PR adds support for the net6.0 TFM in the repo. As part of this, we'll need to update the test environment to use the 6.0 SDK for validation (specifically RC1). Is this a change you're OK with? If so, do you know if it is easy to do with AppVeyor? I'm not super familiar with their CI setup but happy to dig in if you want to bump to 6.0 now.

The (non-ideal) alternative is to wait until 6.0 GAs.

@domaindrivendev
Copy link
Owner

@captainsafia - understood and happy for you to modify appveyor config as needed. It's currently using a pre-built Windows image and I would have expected this to have the .NET 6 preview supported out-of-the-box but according to the docs it only supports up to 5.0.400. So, for now, we may need to install .net 6 with each run by adding an install: section to appveyor.yml

install:
- ps: <Put the powershell command to install $env:DOTNET_VERSION here>

build_script:
- ...

I'm not sure how easy it is to install .NET 6 from the command line so this is something you would have to look into. Looks like you may need to pull down and invoke the dotnet install script

Also, it's worth noting this will slow down every build. If the build speed starts to cause serious issues, we could look into custom build images at a later point. But TBH I'd be surprised if appveyor don't add support for .NET 6 preiews to the canned images fairly soon.

@domaindrivendev
Copy link
Owner

@captainsafia - on a separate note, when I try to build this branch locally using the 6.0.100-preview.7.21379.14 SDK, it fails with the following error:

/Users/richie/src/Swashbuckle.AspNetCore/src/Swashbuckle.AspNetCore.SwaggerGen/SwaggerGenerator/SwaggerGeneratorOptions.cs(89,91): error CS0246: The type or namespace name 'ITagsMetadata' could not be found (are you missing a using directive or an assembly reference?) [/Users/richie/src/Swashbuckle.AspNetCore/src/Swashbuckle.AspNetCore.SwaggerGen/Swashbuckle.AspNetCore.SwaggerGen.csproj]

Which version of the SDK are you using to build locally?

@captainsafia
Copy link
Contributor Author

Which version of the SDK are you using to build locally?

Ah yes, this change was included in RC1 so a nightly installer from https://github.com/dotnet/installer will need to be used. We can always wait until next week when the RC1 SDK is released publicly and do the update thing if you'd like to avoid using a nightly.

@captainsafia captainsafia force-pushed the safia/tags-addition branch 3 times, most recently from 466cf0f to a2b743b Compare September 8, 2021 19:13
@domaindrivendev
Copy link
Owner

domaindrivendev commented Sep 8, 2021

@captainsafia looks good. I just noticed the test failure and think it’s due to the following:

// .Net 5 introduces JsonIgnoreAttribute.Condition which should be honored

Should be an easy fix but makes me wonder if similar issues could be elsewhere in the code. Might be worth reviewing all compiler conditions and make sure they’re all set up to account for future versions as well as current.

@captainsafia
Copy link
Contributor Author

@captainsafia looks good. I just noticed the test failure and think it’s due to the following:

Ah thank you! I thought it was an issue with the new JSON generator. It looks like this is the only ifdef that needed to be updated.

@captainsafia
Copy link
Contributor Author

OK! I think this is in a good state now. If you're cooling taking a dependency on the RC2 nightlies in the build, feel free to merge. Otherwise, we can sit on this until the build is out.

@domaindrivendev
Copy link
Owner

domaindrivendev commented Sep 16, 2021

@captainsafia is it RC1 or RC2 that included this change? I tried building this branch locally with 6.0.100-rc.1 and am getting the same error.

Also, re your question - I'd prefer to wait for a compatible RC (vs nighly build) before merging this if that's OK.

@captainsafia
Copy link
Contributor Author

@domaindrivendev RC2 contains the change which won't be out of nightly for a bit so we can hold this change until then.

In the meantime, this change should work with any SDK version 6.0.100-rc.2.21458.9 or higher.

@domaindrivendev
Copy link
Owner

Sounds good - ping me here as soon as RC2 is out and I'll merge the PR

@captainsafia
Copy link
Contributor Author

@domaindrivendev This should be good for review/merge to main. I've updated it to use the public RC2 bits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants