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

Move clients init file generate to module system [breaking change] #99

Merged
merged 5 commits into from
May 17, 2017
Merged

Move clients init file generate to module system [breaking change] #99

merged 5 commits into from
May 17, 2017

Conversation

ChuntaoLu
Copy link
Contributor

@ChuntaoLu ChuntaoLu commented May 17, 2017 •
edited
Loading

This PR moves the client init file generation into the module system.

Breaking: with this change, a clients-config.json file is required in the clients directory. And to tame the module system, the config file must have following fields as in https://github.com/uber/zanzibar/pull/99/files#diff-4eeffff9fb08e6dff5f0056d5106f422:

{
 	"name": "clients",
 	"type": "init",
 	"config": {}
}

@coveralls
Copy link

coveralls commented May 17, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.1%) to 68.907% when pulling 5d475b9 on lu.init into af1b5e9 on master.

@ChuntaoLu ChuntaoLu changed the title Move clients init file generate to module system Move clients init file generate to module system [breaking change] May 17, 2017
@coveralls
Copy link

coveralls commented May 17, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.06%) to 68.942% when pulling 1694c12 on lu.init into af1b5e9 on master.

@Raynos
Copy link
Contributor

Raynos commented May 17, 2017

@ChuntaoLu

I think @Matt-Esch should be reviewer for this.

I believe he had some plans to generalize the "clients init" without adding a new config file by generating clients.go at build/services/{{service-name}}/clients.go next to build/services/{{service-name}}/main.go

@ChuntaoLu
Copy link
Contributor Author

@Raynos yeah, the config file feels unnecessary, it's currently a hack to tick the module system for a green pass, but might be useful in the future. I requested @Matt-Esch to review the change as well. I want you and @stevededalus to be aware of the breaking change in case this PR goes in.

Matt-Esch
Matt-Esch approved these changes May 17, 2017
View reviewed changes
Copy link
Contributor

@Matt-Esch Matt-Esch left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. We discussed how this is going to change to a more structured approach per binary, but this is a reasonable next step.

codegen/gateway.go Outdated
return nil, errors.Errorf(
"client config (%s) must have customImportPath field for type custom", jsonFile,
)
fields := []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be a declared outside of the function if it is static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done.

codegen/module_system.go Outdated
@@ -134,6 +134,24 @@ func NewDefaultModuleSystem(
)
}

if err := system.RegisterClass("clients", module.Class{
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting way to create a global clients object. You might want to pick a slightly more obvious variant of name here because client vs clients is pretty hard to distinguish at a glance. Nice to see that the SingleModule type actually still works because it isn't used anywhere right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I find it neat to be able to leverage the SingleModule type for the initialization piece. 👍

Regarding the naming, I guess we can keep it as is for now since the config is one time only and doesn't really provide much information.

@coveralls
Copy link

coveralls commented May 17, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 69.309% when pulling 76a362c on lu.init into af1b5e9 on master.

@coveralls
Copy link

coveralls commented May 17, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 69.299% when pulling fa80463 on lu.init into af1b5e9 on master.

Raynos
Raynos approved these changes May 17, 2017
View reviewed changes
Copy link
Contributor

@Raynos Raynos left a comment

Choose a reason for hiding this comment

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

This is a great step towards the end goal.

At some point we can get rid of the "clients" module config.

@ChuntaoLu ChuntaoLu merged commit 3eaf5c7 into master May 17, 2017
@ChuntaoLu ChuntaoLu deleted the lu.init branch May 17, 2017 21:00
@CortezFrazierJr CortezFrazierJr mentioned this pull request Nov 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Raynos Raynos Raynos approved these changes

@Matt-Esch Matt-Esch Matt-Esch approved these changes

@stevededalus stevededalus Awaiting requested review from stevededalus

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@ChuntaoLu @coveralls @Raynos @Matt-Esch