Skip to content

Minimum required changes for working tests #481

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
Merged
merged 16 commits into from
Jan 28, 2021
Merged

Minimum required changes for working tests #481

merged 16 commits into from
Jan 28, 2021

Conversation

dgvives
Copy link
Contributor

@dgvives dgvives commented Jan 11, 2021

Minimum required changes for working tests.
Updated models and broken functionality

@dgvives dgvives mentioned this pull request Jan 12, 2021
Closed
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 reviewed Jan 12, 2021
View reviewed changes
jterry75
jterry75 suggested changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

Some questions

@dgvives dgvives requested a review from jterry75 January 13, 2021 20:02
jterry75
jterry75 suggested changes Jan 14, 2021
View reviewed changes
Copy link
Contributor

@jterry75 jterry75 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 just one question if we can improve the StreamUtil even better here otherwise this looks really good thanks for the contributions

@dgvives dgvives requested a review from jterry75 January 14, 2021 21:18
@dgvives
Copy link
Contributor Author

dgvives commented Jan 14, 2021

@jterry75 I tried your suggestion for StreamUtil and found that ReadAsync(token) wasn't getting any line when token was cancelled.
Approach from #343 fits nicely, is working and validated by running tests.

@The-TT-Hacker
Copy link

This pull request has my minimal changes to update the go code generation to the newer go modules.
Well I guess it's not 100% minimal because it has added the service logs endpoint which was the motivation for updating the code generation.
#472

The-TT-Hacker
The-TT-Hacker reviewed Jan 18, 2021
View reviewed changes
@jterry75
Copy link
Contributor

I'm ok with this at the moment. @galvesribeiro - A second look would be appreciated if you have the time

@galvesribeiro
Copy link
Member

galvesribeiro commented Jan 19, 2021 via email

galvesribeiro
galvesribeiro requested changes Jan 20, 2021
View reviewed changes
Copy link
Member

@galvesribeiro galvesribeiro left a comment

Choose a reason for hiding this comment

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

Just a few tiny changes and we are good.

Thanks for the contribution! I really appreciate that!

jterry75
jterry75 reviewed Jan 20, 2021
View reviewed changes
@dgvives
Copy link
Contributor Author

dgvives commented Jan 21, 2021 •
edited
Loading

@galvesribeiro would you have a look at the CI/CD pipeline and build operations? It might be slightly different to include Directory.Build.props, I can't build or test using 'dotnet build' or 'dotnet test' from command here

Looks like updating Nerbank.GitVersioning package did the trick to compile using 'dotnet build'

galvesribeiro
galvesribeiro requested changes Jan 21, 2021
View reviewed changes
Copy link
Member

@galvesribeiro galvesribeiro left a comment

Choose a reason for hiding this comment

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

Just one last thing.

@galvesribeiro
Copy link
Member

Looks like updating Nerbank.GitVersioning package did the trick to compile using 'dotnet build'

Yeah, was about to say that. Github Actions changed the way the variables are added so they had to update the action to support that. Thanks!

@dgvives dgvives requested a review from galvesribeiro January 21, 2021 13:48
galvesribeiro
galvesribeiro approved these changes Jan 21, 2021
View reviewed changes
Copy link
Member

@galvesribeiro galvesribeiro left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much for the contribution and your patience!

@galvesribeiro
Copy link
Member

@jterry75 Its all good to me. Any final thoughts? Otherwise I'll squash and merge to start the release of a new package.

@dgvives
Copy link
Contributor Author

dgvives commented Jan 22, 2021

Next step: I'd like to order and cleanup the code base, applying Roslyn suggestions and removing warnings, to have it tidy and clean before continuing adding pending contributions/functionality.
No modifications in classes, methods, names, namespaces, none of that whatsoever.

@jterry75
Copy link
Contributor

Sorry been out. @galvesribeiro - I'm good. Lets merge thanks!

@galvesribeiro galvesribeiro merged commit 8e3cf4f into dotnet:master Jan 28, 2021
@jterry75 jterry75 mentioned this pull request Feb 2, 2021
Closed
@ACoderLife ACoderLife mentioned this pull request Sep 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@galvesribeiro galvesribeiro galvesribeiro approved these changes

@jterry75 jterry75 Awaiting requested review from jterry75

+1 more reviewer

@The-TT-Hacker The-TT-Hacker The-TT-Hacker left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dgvives @The-TT-Hacker @jterry75 @galvesribeiro