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

[Feat] Add Advanced Security Properties to Repository Get/Update #2865

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

SlyckLizzie
Copy link
Contributor

@SlyckLizzie SlyckLizzie commented Jan 27, 2024

Resolves #ISSUE_NUMBER
N/A

Before the change?

  • Repository get/update did not implement Advanced Security properties

After the change?

  • Repository get/update implements Advanced Security properties

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@SlyckLizzie
Copy link
Contributor Author

Breaking change is because the current C# language is targeting 7.3 and in order to make a reference object nullable it needs to be targeting at least 8.0. The repository response and update objects will require implementers to add in the SecurityAndAnalysis objects.

@SlyckLizzie SlyckLizzie changed the title Feature/advanced security [Feat] Add Advanced Security Properties to Repository Get/Update Jan 27, 2024
@nickfloyd nickfloyd added Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request labels Feb 1, 2024
@kfcampbell
Copy link
Member

@nickfloyd are you comfortable with this support bump? C# 7.0 came out in 2017, but I can't seem to find any information on the support windows. Some versions of .NET still default to C# 7.x, though I'm not sure if we should still support those.

@nickfloyd
Copy link
Contributor

Breaking change is because the current C# language is targeting 7.3 and in order to make a reference object nullable it needs to be targeting at least 8.0. The repository response and update objects will require implementers to add in the SecurityAndAnalysis objects.

Hey @SlyckLizzie, for clarity reference types in c# have always been nullable. I am guessing you are referring to the enum that was added - status, is that correct?

Enums, which are value types, will allow the nullable operator (since it was introduced in c# 2.0) the only real difference is that we have to do some safety checks like .Value, .HasValue, or string comparison checks.

If you get the chance, would you let me know where in your change set is the modification that will require .NET 8.0? I am sure I am missing it - perhaps with the way we serialize and deserialize things? Let me know so that I can better understand the constraint.

Somewhat related we still have users on this SDK that are still on C# 7.x (LTS) - which is still in maintenance until May 14, 2024 and .NET 6 which is active until November 12, 2024 - we tend to follow the language release cycle for this SDK.

FYI, our generative .NET SDK is targeting .NET 8 as is.

Thanks for this addition, I'm looking forward to getting it in!

@SlyckLizzie
Copy link
Contributor Author

SlyckLizzie commented Mar 13, 2024

@nickfloyd - It's not the framework version but the language version targeted by framework in the project file. netstandard2.0 targets the c# language 7.3. In order for me to be able to make the SecurityAndAnalysisRequest input parameter of the Respository Response model nullable, e.g. SecurityAndAnalysisRequest? the target framework of the project needs to be netstandard2.1 to target the c# language version 8.0

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version

@SlyckLizzie
Copy link
Contributor Author

@nickfloyd - Is there still an issue with this? I noticed a new breaking change version update to the package. Why not add this in with that?

@nickfloyd
Copy link
Contributor

nickfloyd commented Apr 11, 2024

@nickfloyd - Is there still an issue with this? I noticed a new breaking change version update to the package. Why not add this in with that?

Because I completely dropped the ball on this. I'm sorry about that...

We can move forward with what you've suggested. I'll need to roll it out in a new major release (that's on me) but it should be fine. Go ahead and change the target to 2.1 when you get the chance and we can get this out the door.

Also, would you mind bumping Octokit.Reactive as well?

Again, apologies for the delay.

@SlyckLizzie
Copy link
Contributor Author

@nickfloyd - It's all good. I'll make those changes by the end of the weekend. :)

@SlyckLizzie
Copy link
Contributor Author

SlyckLizzie commented Apr 13, 2024

@nickfloyd - Apparently that change isn't going to play nice with the .Net Framework. If it's not .net core or later then it has to reference net standard 2.0. The test and pagnationextension projects still have a reference to 4.62. Everything seems to compile fine without the 4.62 but I want to make sure that's the direction you want to go.

https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-1
image

@nickfloyd
Copy link
Contributor

@nickfloyd - Apparently that change isn't going to play nice with the .Net Framework. If it's not .net core or later then it has to reference net standard 2.0. The test and pagnationextension projects still have a reference to 4.62. Everything seems to compile fine without the 4.62 but I want to make sure that's the direction you want to go.

I was really hoping that all of the projects were going to play well with this change. Let me see if I can determine if 4.6.2 is actually a requirement in those projects or if they can be bumped up as well.

I still believe moving this direction is the right way to go - but it might take a bit more work to get there. Thanks again for the heads up... I'll have a look and let you know what I find / or if you beat me to it - either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants