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

Create event api #1126

Merged
merged 8 commits into from
Jun 27, 2019
Merged

Create event api #1126

merged 8 commits into from
Jun 27, 2019

Conversation

krhubert
Copy link
Contributor

No description provided.

event/event.go Outdated Show resolved Hide resolved
}

// CreateEventRequest defines request for execution update.
message CreateEventRequest {
Copy link
Member

Choose a reason for hiding this comment

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

you could change the message CreateEventRequest to:

message CreateEventRequest {
  // instanceHash is hash of instance that can proceed an execution.
  string instanceHash = 1;

   // key is the key of the event.
  string key = 2;

   // data is the data for the event.
  string data = 3;
}

So there is not need to check for a not empty hash in server/grpc/api/event.go

Also I think it's more logic to have different messages to create the resource and to return it.
The same modification should be done (but in a future time) on ServiceX/Create. Reusing the definition.service causes more problem than it solves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// The request's data for the `Create` API.
message CreateServiceRequest {
  definition.Service definition = 1; // The service definition to use to create the Service.
}

We accept service in servcie api, so I wanted to be consistent. And I prefer to check if hash is empty rather then redefine all event fields (same for CreateServiceRequest). It's more clear that CreateEventRquest expects event as argument.

Copy link
Member

Choose a reason for hiding this comment

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

The following issue is actually to change CreateServiceRequest: #1111

server/grpc/api/event.go Outdated Show resolved Hide resolved
@@ -31,26 +31,25 @@ func New(ps *pubsub.PubSub, service *servicesdk.Service, instance *instancesdk.I
}

// Emit emits a MESG event eventKey with eventData for service token.
func (e *Event) Emit(instanceHash hash.Hash, eventKey string, eventData map[string]interface{}) error {
func (e *Event) Emit(instanceHash hash.Hash, eventKey string, eventData map[string]interface{}) (*event.Event, error) {
Copy link
Member

@NicolasMahe NicolasMahe Jun 26, 2019

Choose a reason for hiding this comment

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

Suggested change
func (e *Event) Emit(instanceHash hash.Hash, eventKey string, eventData map[string]interface{}) (*event.Event, error) {
func (e *Event) Emit(evnt *event.Event) (error) {

what about changing the parameters of this function to directly accept the event struct?
I think it will make this function a bit simpler as well as the api package.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine like that, otherwise, we will still have the confusion between the actual inputs and the resource itself which is not good. We will have the same problem that we have with the service.create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ok with both - just another thing we can make consistent accross source code.

Copy link
Member

@NicolasMahe NicolasMahe Jun 27, 2019

Choose a reason for hiding this comment

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

The function create in serviceSDK accept directly the service:

func (s *Service) Create(srv *service.Service) (*service.Service, error) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I upadte this when I'm going to remove coreapi methods?

protobuf/api/event.proto Outdated Show resolved Hide resolved
protobuf/api/event.proto Outdated Show resolved Hide resolved
@@ -31,26 +31,25 @@ func New(ps *pubsub.PubSub, service *servicesdk.Service, instance *instancesdk.I
}

// Emit emits a MESG event eventKey with eventData for service token.
func (e *Event) Emit(instanceHash hash.Hash, eventKey string, eventData map[string]interface{}) error {
func (e *Event) Emit(instanceHash hash.Hash, eventKey string, eventData map[string]interface{}) (*event.Event, error) {
Copy link
Member

Choose a reason for hiding this comment

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

It's fine like that, otherwise, we will still have the confusion between the actual inputs and the resource itself which is not good. We will have the same problem that we have with the service.create

krhubert and others added 2 commits June 26, 2019 18:58
@NicolasMahe NicolasMahe merged commit a5825ec into dev Jun 27, 2019
@NicolasMahe NicolasMahe deleted the feature/protobuf-api branch June 27, 2019 10:33
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.

3 participants