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

Filter instances on backend side #1491

Merged
merged 12 commits into from
Nov 14, 2019
Merged

Filter instances on backend side #1491

merged 12 commits into from
Nov 14, 2019

Conversation

krhubert
Copy link
Contributor

@krhubert krhubert commented Nov 8, 2019

clsoes #1467

@krhubert krhubert added the enhancement New feature or request label Nov 8, 2019
@krhubert krhubert requested a review from NicolasMahe November 8, 2019 09:33
@krhubert krhubert self-assigned this Nov 8, 2019
@krhubert krhubert added this to the next milestone Nov 8, 2019
sdk/instance/backend.go Outdated Show resolved Hide resolved
sdk/instance/sdk.go Show resolved Hide resolved
sdk/instance/sdk.go Outdated Show resolved Hide resolved
sdk/instance/backend.go Outdated Show resolved Hide resolved
@krhubert krhubert force-pushed the feature/filter-instance branch from a3c0931 to bedd3e0 Compare November 8, 2019 18:56
@krhubert
Copy link
Contributor Author

krhubert commented Nov 8, 2019

I don't know why e2e fails, can you help me fix that?

cosmos/client.go Outdated Show resolved Hide resolved
@NicolasMahe
Copy link
Member

NicolasMahe commented Nov 11, 2019

I don't know why e2e fails, can you help me fix that?

servicesdk.hash is using proto.Marshal.

// Hash returns the calculate hash of a service.
func (s *SDK) Hash(req *api.CreateServiceRequest) (hash.Hash, error) {
	var h hash.Hash
	b, err := proto.Marshal(req)
	if err != nil {
		return nil, err
	}
	if err := s.client.Query("custom/"+backendName+"/hash", cmn.HexBytes(b), &h); err != nil {
		return nil, err
	}
	return h, nil
}

EDIT: i fixed it

sdk/service/backend.go Outdated Show resolved Hide resolved
sdk/instance/backend.go Outdated Show resolved Hide resolved
sdk/instance/codec.go Outdated Show resolved Hide resolved
sdk/service/codec.go Outdated Show resolved Hide resolved
@krhubert krhubert requested a review from NicolasMahe November 13, 2019 09:33
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.

Few issues while testing:

Crash

grpcurl -proto protobuf/api/instance.proto -plaintext localhost:50052 mesg.api.Instance/List

This crashes

panic: MarshalBinaryBare cannot marshal a nil pointer directly. Try wrapping in a struct?

Weird behavior with the filter

That might be related to grpcurl or base64 conversion but if I use the filter with slightly different values it still matches when it shouldn't

grpcurl -proto protobuf/api/instance.proto -plaintext -d "{\"filter\": {}}" localhost:50052 mesg.api.Instance/List
➜  engine git:(feature/filter-instance) grpcurl -proto protobuf/api/instance.proto -plaintext -d "{\"filter\": {}}" localhost:50052 mesg.api.Instance/List
{
  "instances": [
    {
      "hash": "r1gIofZs2MZQVBsm6TGp5MGEoafY148TGNRJEnkvn1o=",
      "serviceHash": "g6BhSw8QiSibfNkIIdkc5QRnhJVo4GJfE8QPcjKVP3I=",
      "envHash": "T1PNoYwrqgwDVLtfmj7L5e0Sq02OEbqHPC8RFhICuUU="
    }
  ]
}

# valid 
grpcurl -proto protobuf/api/instance.proto -plaintext -d "{\"filter\": {\"serviceHash\": \"g6BhSw8QiSibfNkIIdkc5QRnhJVo4GJfE8QPcjKVP3I=\"}}" localhost:50052 mesg.api.Instance/List

# still valid by changing the last letter `I` per `J`.
grpcurl -proto protobuf/api/instance.proto -plaintext -d "{\"filter\": {\"serviceHash\": \"g6BhSw8QiSibfNkIIdkc5QRnhJVo4GJfE8QPcjKVP3J=\"}}" localhost:50052 mesg.api.Instance/List

@krhubert krhubert force-pushed the feature/filter-instance branch from cfbebe2 to c844f60 Compare November 14, 2019 08:30
@krhubert
Copy link
Contributor Author

@antho1404

Fixed issues with panic. Also add e2e tests for different instance hash and it works as expected. I'm not going to debug grpcurl as it is just a helper tool. From e2e tests and code perespective everything work fine.

@krhubert krhubert requested a review from antho1404 November 14, 2019 08:41
@NicolasMahe NicolasMahe merged commit fc58b1a into dev Nov 14, 2019
@NicolasMahe NicolasMahe deleted the feature/filter-instance branch November 14, 2019 09:01
@NicolasMahe NicolasMahe added the release:change Pull requests that change something existant label Nov 26, 2019
@NicolasMahe NicolasMahe mentioned this pull request Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release:change Pull requests that change something existant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants