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

service: add Getters and Validate, Require methods for task, event and output #409

Merged
merged 3 commits into from
Aug 30, 2018

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Aug 29, 2018

resolves #349
resolves #348
related with #337

@ilgooz ilgooz added this to the v1.2.0 milestone Aug 29, 2018
@ilgooz ilgooz self-assigned this Aug 29, 2018
@ilgooz ilgooz requested a review from a team August 29, 2018 16:04
execution/complete.go Show resolved Hide resolved
api/deploy_deployer.go Show resolved Hide resolved
event/event.go Outdated Show resolved Hide resolved
@ilgooz ilgooz force-pushed the feature/service-getters branch 2 times, most recently from 54011d6 to ed224b7 Compare August 29, 2018 22:12
string key = 4; // Event's key.
string name = 1; // Event's name.
string description = 2; // Event's description.
string serviceName = 5; // Event's service name.
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to keep serviceName in Event in the future?
If not, how do you plan to access this data in the future from the Event type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no i don't. i'll add private service field to Event type.

type Event struct {
   service *Service
}

Copy link
Member

Choose a reason for hiding this comment

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

I really don't like to have the service in the event, we should try to keep things really separated and ask ourselves why we really need the service in the event. If it's just to have the name of the service in one error this can be resolved with a event error that is catch and where the calling function can add the service.
Have a double dependencies between resources always makes things way more complicated in the end

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.

I understand that this PR is temporary, to be able to have our own structure for service. Just one thing I want to point:

The service.proto is not really needed when we have our own structure because the deployment is done by tar or url and the only api that use service.proto are list and get and they can have their own data, they don't need to send all the service.

@antho1404 antho1404 merged commit d191696 into dev Aug 30, 2018
@antho1404 antho1404 deleted the feature/service-getters branch August 30, 2018 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants