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

feat(Core): CNX-8570, CNX-8393 Transporter exception cleanup and IsFatal(this Exception) function #3114

Merged
merged 7 commits into from
Jan 4, 2024
Merged

feat(Core): CNX-8570, CNX-8393 Transporter exception cleanup and IsFatal(this Exception) function #3114

merged 7 commits into from
Jan 4, 2024

Conversation

JR-Morgan
Copy link
Member

@JR-Morgan JR-Morgan commented Dec 18, 2023 •
edited

Lets merge #3109 first as this one is built off the last

@JR-Morgan JR-Morgan changed the base branch from main to dev December 18, 2023 23:53
@JR-Morgan JR-Morgan changed the base branch from dev to jrm/core/warningsagain December 18, 2023 23:54
JR-Morgan
JR-Morgan commented Dec 18, 2023
View reviewed changes
Core/Core/Models/Base.cs Outdated Show resolved Hide resolved
@JR-Morgan JR-Morgan changed the title Core: Transporter exception cleanup and IsFatal(this Exception) function feat(Core): Transporter exception cleanup and IsFatal(this Exception) function Dec 19, 2023
@JR-Morgan JR-Morgan marked this pull request as ready for review December 19, 2023 00:03
@JR-Morgan JR-Morgan added the core issues related to the .net sdk. label Dec 19, 2023
@JR-Morgan JR-Morgan added this to the 2.18 milestone Dec 19, 2023
@JR-Morgan JR-Morgan changed the title feat(Core): Transporter exception cleanup and IsFatal(this Exception) function feat(Core): CNX-8570 Transporter exception cleanup and IsFatal(this Exception) function Dec 19, 2023
@AlanRynne AlanRynne mentioned this pull request Dec 19, 2023
Merged
Base automatically changed from jrm/core/warningsagain to dev December 19, 2023 17:08
@AlanRynne
Copy link
Member

Merged! I think we need a rebase of this now :)

@JR-Morgan
Copy link
Member Author

JR-Morgan commented Dec 19, 2023 •
edited

Back merge from dev complete. Ready for review
Back merge wasn't clean enough for Alan 😁, so done a squash + cherry pick

@JR-Morgan JR-Morgan force-pushed the jrm/core/transport-cleanup branch 3 times, most recently from 19f0bc1 to 3404b73 Compare December 19, 2023 18:17
@JR-Morgan
Transport exception handling
46885d3 
fix(core): No wrapping of fatal exceptions

fixed warnings
AlanRynne
AlanRynne previously requested changes Dec 19, 2023
View reviewed changes
Copy link
Member

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

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

Mostly great! 🙌🏼 I have questions, pinged @BovineOx to weigh in on some, and some requests/opinionated thoughts

Core/Core/Credentials/AccountManager.cs Outdated Show resolved Hide resolved
Core/Core/Credentials/StreamWrapper.cs Show resolved Hide resolved
Core/Core/Helpers/Http.cs Show resolved Hide resolved
Core/Core/Helpers/Http.cs Show resolved Hide resolved
Core/Core/Logging/ExceptionHelpers.cs Show resolved Hide resolved
Core/Core/Models/Base.cs Outdated Show resolved Hide resolved
Core/Core/Serialisation/DeserializationWorkerThreads.cs Show resolved Hide resolved
Core/Core/Transports/ITransport.cs Outdated Show resolved Hide resolved
Core/Core/Transports/TransportExtensions.cs Outdated Show resolved Hide resolved
Core/Tests/Kits/KitManagerTests.cs Outdated Show resolved Hide resolved
@BovineOx
Copy link
Contributor

@JR-Morgan I think it's broadly fine - a couple of issues we should consider but if you can raise a ticket and reference that in those places in the code and then it can go.

@JR-Morgan JR-Morgan changed the title feat(Core): CNX-8570 Transporter exception cleanup and IsFatal(this Exception) function feat(Core): CNX-8570 CNX-8393Transporter exception cleanup and IsFatal(this Exception) function Jan 4, 2024
@JR-Morgan JR-Morgan changed the title feat(Core): CNX-8570 CNX-8393Transporter exception cleanup and IsFatal(this Exception) function feat(Core): CNX-8570 CNX-8393 Transporter exception cleanup and IsFatal(this Exception) function Jan 4, 2024
@JR-Morgan JR-Morgan changed the title feat(Core): CNX-8570 CNX-8393 Transporter exception cleanup and IsFatal(this Exception) function feat(Core): CNX-8570, CNX-8393 Transporter exception cleanup and IsFatal(this Exception) function Jan 4, 2024
BovineOx
BovineOx reviewed Jan 4, 2024
View reviewed changes
Core/Core/Helpers/Http.cs Outdated Show resolved Hide resolved
@JR-Morgan JR-Morgan removed the request for review from AlanRynne January 4, 2024 16:51
@JR-Morgan JR-Morgan dismissed AlanRynne’s stale review January 4, 2024 16:53

Comments resolved. @BovineOx has given a thumbs up in light of Alan being on holiday

@JR-Morgan JR-Morgan merged commit 614ebe2 into dev Jan 4, 2024
30 checks passed
@JR-Morgan JR-Morgan deleted the jrm/core/transport-cleanup branch January 4, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlanRynne AlanRynne AlanRynne left review comments

@BovineOx BovineOx BovineOx approved these changes

Assignees
No one assigned
Labels
core issues related to the .net sdk.
Projects
None yet
Milestone
2.18
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JR-Morgan @AlanRynne @BovineOx