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

Upgrade .NET from 6.0 to 8.0 #3046

Merged
merged 24 commits into from
Jun 14, 2024
Merged

Upgrade .NET from 6.0 to 8.0 #3046

merged 24 commits into from
Jun 14, 2024

Conversation

imnasnainaec
Copy link
Collaborator

@imnasnainaec imnasnainaec commented Apr 8, 2024

Follows #3049, #3050, #3052
Resolves #3043


This change is Reviewable

@imnasnainaec imnasnainaec added backend deployment dependencies Pull requests that update a dependency file labels Apr 8, 2024
@imnasnainaec imnasnainaec self-assigned this Apr 8, 2024
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.14%. Comparing base (8ca63d9) to head (d17beea).

Files Patch % Lines
Backend/Services/InviteService.cs 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3046      +/-   ##
==========================================
+ Coverage   74.98%   75.14%   +0.16%     
==========================================
  Files         273      273              
  Lines       10397    10386      -11     
  Branches     1230     1230              
==========================================
+ Hits         7796     7805       +9     
+ Misses       2241     2223      -18     
+ Partials      360      358       -2     
Flag Coverage Δ
backend 84.33% <66.66%> (+0.36%) ⬆️
frontend 66.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jasonleenaylor

This comment was marked as resolved.

@jasonleenaylor

This comment was marked as resolved.

@jasonleenaylor

This comment was marked as resolved.

This comment has been minimized.

@imnasnainaec imnasnainaec marked this pull request as ready for review June 6, 2024 15:09
@jasonleenaylor
Copy link
Contributor

docs/user_guide/assets/licenses/backend_licenses.txt line 34 at r1 (raw file):

####################################################################################################
Package:icu.net

? Any thoughts on why this went away?

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

I think there might be one more TryGetValue where we don't use the out value that I missed. Otherwise this is looking good.

Reviewed 38 of 42 files at r1, 3 of 5 files at r2, 1 of 20 files at r5, 2 of 11 files at r6, 1 of 1 files at r8.
Reviewable status: 12 of 43 files reviewed, all discussions resolved (waiting on @imnasnainaec)


.github/workflows/backend.yml line 121 at r1 (raw file):

            github.com:443
            objects.githubusercontent.com:443
            ts-crl.ws.symantec.com:80

what? Hmm, codeql hits this? weird.

Code quote:

ts-crl.ws.symantec.com:80

Backend/Controllers/InviteController.cs line 95 at r1 (raw file):

                {
                    currentUser = user;
                    if (!user.ProjectRoles.TryGetValue(projectId, out var _roleId))

This one doesn't make sense to me. Why is it better to get a value we don't use?


Backend/Controllers/ProjectController.cs line 57 at r1 (raw file):

            var allUsers = await _userRepo.GetAllUsers();
            var projectUsers = allUsers.FindAll(user => user.ProjectRoles.TryGetValue(projectId, out var _roleId));

Also here, why get the value and not use it - this suggestion doesn't make sense to me.


Backend/Services/StatisticsService.cs line 74 at r1 (raw file):

            var allUsers = await _userRepo.GetAllUsers();
            var projectUsers = allUsers.FindAll(user => user.ProjectRoles.TryGetValue(projectId, out var _roleId));

I prefer ContainsKey if we don't go on to lookup the value.

Copy link
Collaborator Author

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewable status: 12 of 43 files reviewed, all discussions resolved (waiting on @jasonleenaylor)


docs/user_guide/assets/licenses/backend_licenses.txt line 34 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

? Any thoughts on why this went away?

That's puzzling. It disappears when I run on my Windows so I reran on Mac and got it back.

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 42 files at r1, 2 of 5 files at r2, 7 of 7 files at r3, 16 of 20 files at r5, 8 of 11 files at r6, 3 of 4 files at r7, 1 of 1 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)

@imnasnainaec imnasnainaec merged commit b4d3809 into master Jun 14, 2024
18 checks passed
@imnasnainaec imnasnainaec deleted the dotnet8 branch June 14, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend dependencies Pull requests that update a dependency file deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to .NET 8.0
3 participants