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.

Sign up for GitHub

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

Jump to bottom

Pc 1122 investigation into c# and .net update #293

Merged
merged 6 commits into from
Jun 24, 2024
Merged

Pc 1122 investigation into c# and .net update #293

merged 6 commits into from
Jun 24, 2024

Conversation

Glenn-Clarke
Copy link
Collaborator

Link to Jira ticket

Description

C# & .NET update changes with port number 8080 reverted to 80

Checklist

  • I have made any necessary updates to the documentation
  • I have checked there are no unnecessary IDE warnings in this PR
  • I have checked there are no unintentional line ending changes
  • I have added tests where applicable
  • If I have made any changes to the code, I have used the IDE auto-formatter on it
  • If I have made any changes to website flow, I have checked forward and back behaviour is still consistent
  • If I have made any changes to content strings or resource files, I have followed the documentation

@Glenn-Clarke
PC-1122: Investigation results. Updated references to .net from 6.0 t…
844c3a0 
…o 8.0, and C# from 10 to 11.

Change ports to 8080 from 80. Update SDK from 5.0 to 8.0.

(cherry picked from commit c4ca52a)
@Glenn-Clarke
PC-1122: Updated C# to 12. Reverted CLI SDK to 5.0
f3a911f 
(cherry picked from commit bc13349)
@Glenn-Clarke
PC-1122: Updated .Net CLI to 8.0
dd16c20 
(cherry picked from commit 64d43d7)
@Glenn-Clarke
PC-1050: Reverted port number changes
c4679a5
@Glenn-Clarke Glenn-Clarke requested a review from jdgage June 21, 2024 09:45
jdgage
jdgage reviewed Jun 21, 2024
View reviewed changes
Copy link
Contributor

@jdgage jdgage left a comment

Choose a reason for hiding this comment

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

Could you have a quick look into the (I think new with .NET 8) build warnings? i.e. whether they're something that we can easily deal with now?

docker-compose.yml
@@ -7,6 +7,7 @@ services:
- "5001:80"
environment:
ASPNETCORE_ENVIRONMENT: "Development"
ASPNETCORE_HTTP_PORTS: "80"
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming this all works fine running locally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested this change yesterday after you suggested trying it locally, and it appears to work fine, including PDF generation.

@Glenn-Clarke
Copy link
Collaborator Author

Looks like the build warnings were simple replacements, but I'm testing the changes locally now

jdgage
jdgage previously approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@jdgage jdgage left a comment

Choose a reason for hiding this comment

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

Did you look at the warning about lower-cased characters in that migration class name? I think we should be able to just rename the class (and maybe add a comment about why)

@Glenn-Clarke
Copy link
Collaborator Author

They don't seem to come up in the build... I'll have a look now quickly and see if I can find them

@Glenn-Clarke
Copy link
Collaborator Author

They didn't show up in my IDE unless I went to file in question 😓 I found them using the github build warnings, and I've fixed it now. I renamed the file and class for the migrations and the data in my local db seems to be fine. Hope this is ok!

@jdgage
Copy link
Contributor

jdgage commented Jun 24, 2024

I think we only needed to rename the class, but probably still fine, so let's leave it now. If you want reassurance: try docker compose up before the rename and again after and make sure it doesn't try to reapply the migrations. But we'll find out soon enough when it deploys to dev.

@Glenn-Clarke
Copy link
Collaborator Author

Docker compose up seems to be ok? I think?

@Glenn-Clarke Glenn-Clarke merged commit 71b1629 into dev Jun 24, 2024
2 checks passed
@Glenn-Clarke Glenn-Clarke mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdgage jdgage jdgage approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Glenn-Clarke @jdgage