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

introduce Newable service #417

Merged
merged 23 commits into from
Sep 12, 2018
Merged

introduce Newable service #417

merged 23 commits into from
Sep 12, 2018

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Sep 1, 2018

depends on #414.

@ilgooz ilgooz self-assigned this Sep 1, 2018
@ilgooz ilgooz added this to the v1.2.0 milestone Sep 1, 2018
@ilgooz ilgooz force-pushed the feature/service-new branch 5 times, most recently from ee1072f to 8184f7c Compare September 2, 2018 07:56
@ilgooz
Copy link
Contributor Author

ilgooz commented Sep 2, 2018

prevent cycling references if possible /cc @antho1404

@ilgooz ilgooz force-pushed the feature/service-new branch 7 times, most recently from 15ff4c3 to 08ddceb Compare September 4, 2018 03:58
* use slices instead maps on Service type, closes #394.
* use slices instead maps on service.proto.
* add ServiceDefinition type to service/importer.
* rm unneeded fields in service.proto in interface/grpc/core.
* rm unneeded service.DependenciesFromService(), closes #393.
* rm unneeded TestDependenciesFromService test in service/.
* rm unneeded TestSaveReturningHash test in database/services.
* rm unneeded TestInjectConfigurationInDependencies test in api/, TestNew test in service/ covers it.
* move TestInjectConfigurationInDependenciesWithConfig test as TestInjectDefinitionWithConfig from api/ to service/.
* move TestInjectConfigurationInDependenciesWithDependency test as TestInjectDefinitionWithDependency from api/ to service/.
* rm unneeded TestInjectConfigurationInDependenciesWithDependencyOverride test in api/.
* use getters in service package for other needed places.
* rm unnecessary nil checks.
* cleanup service package.
* minor improvements.
Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

We don't need to implement this now, these are mostly notes but nothing blocking for the merge

Too many services

In many places we need to do a service.FromService(&service.Service{}). It's not really clear, I recommend to use Definition service.FromDefinition(&service.Definition{})
Same with the fromService() => fromDefinition()

Reminder

We will need to update the Go and JS library

Service in sub-structs

Event, Outputs etc... have a reference on the service and the only reason they have this is to have a nice error message. The service is never needed in the actual business logic, it's just user cosmetic so I would recommend to get rid of this because it's not really needed for the business logic and it can make things complicated in the future

@ilgooz
Copy link
Contributor Author

ilgooz commented Sep 9, 2018

after some investigation we realized that problem with the service list command was due to a leftover mesg-core docker volume that has some services with the old version of Service definition. everything is fine after volume is destroyed.

@ilgooz
Copy link
Contributor Author

ilgooz commented Sep 12, 2018

good to review

x/xstrings/strings.go Outdated Show resolved Hide resolved
NicolasMahe
NicolasMahe previously approved these changes Sep 12, 2018
@antho1404 antho1404 merged commit fad98be into dev Sep 12, 2018
@antho1404 antho1404 deleted the feature/service-new branch September 12, 2018 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants