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

Fixed tests and included pending unmerged feature PRs #480

Closed
wants to merge 29 commits into from
Closed

Fixed tests and included pending unmerged feature PRs #480

wants to merge 29 commits into from

Conversation

dgvives
Copy link
Contributor

@dgvives dgvives commented Jan 4, 2021

@dnfadmin
Copy link

dnfadmin commented Jan 4, 2021

CLA assistant check
All CLA requirements met.

David García Vives added 7 commits January 4, 2021 15:51
System.AggregateException : One or more errors occurred. (Docker API responded with status code=InternalServerError, response={"message":"rpc error: code = Unknown desc = raft: failed to process the request: node lost leader status
@jterry75
Copy link
Contributor

jterry75 commented Jan 4, 2021

@dgvives - More than happy to take some of these changes. Would you be willing to submit separate PR's for more of the systemic changes. Ex:

Conversion from = default(Type) to = default
Conversion from this._name to _name
etc.

It makes it easier to get these out of the way and then we can review just the real changes.

@dgvives
Copy link
Contributor Author

dgvives commented Jan 4, 2021

Reply to #480 (comment) by @jterry75

Agreed, I'll leave out the non functional changes. On next submission should be almost ready for revision

@dgvives dgvives marked this pull request as ready for review January 4, 2021 21:46
dgvives and others added 3 commits January 4, 2021 23:22
specgen README formatting
…esAsync

      System.ArgumentException : Stream was not readable.
@jterry75
Copy link
Contributor

jterry75 commented Jan 6, 2021

Hey dont fully understand the godeps removal set of changes. Do you want to explain that so I can understand the model changes you made as well?

@dgvives
Copy link
Contributor Author

dgvives commented Jan 6, 2021

Reply to #480 (comment) by @jterry75

Changes in godeps come from #472

Updated the specgen tool so it uses the newer go modules instead of godep.
All you have to do to update the models is install go, update the docker repo which is imported using go get -u github.com/docker/docker@ and then run the update-generate-code.cmd script. It downloads all of the correct dependencies, builds the program and generates the models. This has been updated in the readme for specgen.

This retrieves model references for the specified tag from github to local go path, so it is no longer required to include all vendor files within the repository.

After applying these changes I updated models:

go get -u github.com/docker/docker@v20.10.1
.\update-generated-code.cmd

Because some models don't have rest tags, like rest:"query", these had to be specified in modeldefs with proper rest tags for model generation. I reviewed all the models and for the ones with specification being overridden in modeldefs file I specified the proper tags using latest go API v20.10.1

All the generated models come from specgen.go and modeldefs.go files

Just noticed go.mod file wasn't committed, I'm including it now.

@dgvives dgvives requested a review from jterry75 January 8, 2021 11:56
@The-TT-Hacker
Copy link

@dgvives it would have been nice to merge my and a number of other people's changes into your branch rather than copy the changes so that the commit log has a record of who wrote what code. It makes it really hard to review a humongous PR on its own.

@jterry75
Copy link
Contributor

@dgvives - I'm sorry for the additional ask here.

Can you please submit a "formatting" PR separate from this one that cleans up all tabs to spaces, removes trailing spaces, fixes comment alignment etc.

Second, updating the model should be done in a separate PR so that its easy to review "generated" changes since they are really a stamp of approval and dont need a lot of thought. That will remove 90% of the changes from this PR as well.

Then we can review the actual important changes here. I appreciate the work its just really hard to manage this many changes all at once

@dgvives
Copy link
Contributor Author

dgvives commented Jan 11, 2021

@jterry75 I'm working on a new changeset #481 with the minimum required changes for tests to work. Additionally, it looks like VSCode is formatting the code, adding some spaces here and there, I hope this to not be a issue.

The smallest changeset it's going to be big, but let's try this.

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.

Events monitoring stream does not seems to always include CR or LF character at the end of every messages
4 participants