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

ASP.NET Core 3.1 support #14

Merged
merged 6 commits into from
Aug 24, 2021
Merged

Conversation

ddunkin
Copy link
Contributor

@ddunkin ddunkin commented Oct 30, 2020

No description provided.

Having a transitive dependency on Microsoft.AspNetCore.Mvc.Api.Analyzers causes build failures for projects that reference LtiAdvantage and a newer version of AspNetCore.
@jmpease
Copy link
Contributor

jmpease commented Oct 30, 2020

Full disclosure I'm a bit naive on the subject, but wonder what is gained by retargeting to netcoreapp3.1? My understanding is that the current netstandard2.0 target makes this library consumable across a wider range of framework impls and versions?

Unfortunately we've backed ourselves into a corner where it will take some significant effort to upgrade to 3.1 in our current project. Is it possible to maintain a separate branch supporting netstandard2.0...or is this a case for multi-targeting? Happy to assist in the effort if anyone can provide some insight on best approach here.

@ddunkin
Copy link
Contributor Author

ddunkin commented Oct 31, 2020

ASP.NET Core targets netcoreapp3.1 now, and the AssignmentGradeServices and NamesRoleProvisioningService have dependencies on Microsoft.AspNetCore packages. I briefly investigated splitting those components out into separate packages, which seemed like it would work, but was a more invasive change. Mutitageting is another option if 2.x support needs to be maintained; a couple #ifdefs would cover that.

Are you still using 2.2 even though it’s EOL? I had a couple projects I had to decided to either move up to 3.1 or back down to 2.1 to stay on a framework that would still get security updates. Fortunately they were simple, so I was able to upgrade them. It’s a bummer the 2.2 to 3.x upgrade path is difficult in some cases.

@jmpease
Copy link
Contributor

jmpease commented Nov 2, 2020

Are you still using 2.2 even though it’s EOL?

Unfortunately. A significant portion of our dependencies target full framework so we've been facing the same decision. We'll likely end up moving back to 2.1 for security updates and then begin the refactoring in order to move forward.

@ddunkin
Copy link
Contributor Author

ddunkin commented Nov 2, 2020

Are you still using 2.2 even though it’s EOL?

Unfortunately. A significant portion of our dependencies target full framework so we've been facing the same decision. We'll likely end up moving back to 2.1 for security updates and then begin the refactoring in order to move forward.

I'll update this PR to multi-target 2.1 and 3.1.

Target netstandard2.0 and netcoreapp3.1 for LtiAdvantage and netcoreapp2.1 and netcoreapp3.1 for tests.
@ddunkin
Copy link
Contributor Author

ddunkin commented Dec 14, 2020

@srijken, can you (or another committer) review this please?

@ddunkin
Copy link
Contributor Author

ddunkin commented Jan 7, 2021

@andyfmiller, can you review this please?

Copy link
Collaborator

@andyfmiller andyfmiller left a comment

Choose a reason for hiding this comment

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

Well done!

@@ -45,6 +45,7 @@ public async void AddScore_WhenScopeAllows(string scope, HttpStatusCode statusCo

_client.DefaultRequestHeaders.Add("x-test-scope", scope);
var response = await _client.PostAsync(ScoresUrl, scoreContent);
var responseContent = await response.Content.ReadAsStringAsync();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line could be removed.

Copy link
Collaborator

@andyfmiller andyfmiller left a comment

Choose a reason for hiding this comment

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

Thank you Dave!

@ddunkin
Copy link
Contributor Author

ddunkin commented Jan 12, 2021

What's the process for getting this merged?

@paultechguy
Copy link

Just thought I'd check in and see if there are plans to have the 3.1 changes merged into master. ??

@srijken srijken merged commit 5819567 into LtiLibrary:master Aug 24, 2021
@srijken
Copy link
Contributor

srijken commented Aug 24, 2021

Sorry about the delay in the merge!

@ddunkin
Copy link
Contributor Author

ddunkin commented Aug 24, 2021

Thanks!

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.

5 participants