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

Converted to mod, using jsonpb to marshal stub responses and fixed tests #62

Merged
merged 4 commits into from
Feb 15, 2021
Merged

Converted to mod, using jsonpb to marshal stub responses and fixed tests #62

merged 4 commits into from
Feb 15, 2021

Conversation

HairyMike
Copy link
Contributor

@HairyMike HairyMike commented Nov 12, 2020

I've updated the master branch on my fork with these changes:
https://github.com/hairymike/gripmock

And created a docker image:
https://hub.docker.com/r/hairymike/gripmock

docker pull hairymike/gripmock

First of all, great work on gripmock!

It would be great to start upgrading some of the other libraries in gripmock. This PR would lay some of the groundwork for this by first changing over go modules, and replacing packr2 with pkger.

There was a problem running go mod init when using packr2, so I went ahead and migrated it.

Included in this PR:

  • switched over to go modules
  • replaced packr2 with pkger
  • fixes to address the well_known_type test failure. The cause of the issue was that the keyword type was making it into an import alias in the server.tmpl file.
  • migrated from depreciated protoc plugin lib to protogen
  • use jsonpb to marshal stubs to response messages

Few issues that this might resolve:
#61
#58
#51

@HairyMike HairyMike changed the title Converted to mod and migrated from packr2 to pkger Converted to mod, migrated from packr2 to pkger and fixed tests Nov 12, 2020
@HairyMike HairyMike changed the title Converted to mod, migrated from packr2 to pkger and fixed tests Converted to mod, using jsonpb to marshal stub responses and fixed tests Nov 13, 2020
@jekiapp
Copy link
Contributor

jekiapp commented Feb 10, 2021

Thankyou for this well written PR 👍 . Sorry for the late response, I will try to find a time this week to detailed check of this PR.

Copy link
Contributor

@jekiapp jekiapp left a comment

Choose a reason for hiding this comment

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

In the main() @generator.go, what you want to introduce there?
what is the benefit of using pluginpb.codeGeneratorRequest instead of generator.Request?

does the old lib is deprecated? because the updated code seems less simplistic than previous one.

}

gen.CommandLineParameters(gen.Request.GetParameter())
params := make(map[string]string)
Copy link
Contributor

Choose a reason for hiding this comment

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

previously we have this one line gen.CommandLineParameters(gen.Request.GetParameter()) and now we had to manually parse the parameter, care to elaborate?

Copy link
Contributor Author

@HairyMike HairyMike Feb 11, 2021

Choose a reason for hiding this comment

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

I agree,

The reason for the change is that the original generator lib is deprecated, and is not covered by the protocol buffer compatibility guarantee anymore (more details here).

I think there may have been some issue using generator with protogen as well (I can't remember). I'm happy to have a look at backing this part out if its a deal breaker

Thanks for having a look at my PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thankyou for your detailed explanation. PR accepted :)

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.

2 participants