search

allegro / marathon-consul

Integrates Marathon apps with Consul service discovery.
Apache License 2.0
191 stars 33 forks source link

Change FindTaskById function #298

Closed JurrianFahner closed 1 year ago

JurrianFahner commented 4 years ago

This change will fix issue: https://github.com/allegro/marathon-consul/issues/296

It makes the FindTaskById function to match also on the first part of an id, so marathon-consul will be compatible with version 1.9 of marathon.

To avoid matching on too little information, also the requirement of at least a length of an id with 36 chars.

JurrianFahner commented 4 years ago

@ojagodzinski The build is failing, how can it fixed? See below output of Travis. The package build succeeds now.

$ goveralls -coverprofile=coverage/gover.coverprofile -service travis-ci
Error parsing coverage: open coverage/gover.coverprofile: no such file or directory
The command "goveralls -coverprofile=coverage/gover.coverprofile -service travis-ci" exited with 1.
Done. Your build exited with 1.
ojagodzinski commented 4 years ago

@ojagodzinski The build is failing, how can it fixed? See below output of Travis. The package build succeeds now.

$ goveralls -coverprofile=coverage/gover.coverprofile -service travis-ci
Error parsing coverage: open coverage/gover.coverprofile: no such file or directory
The command "goveralls -coverprofile=coverage/gover.coverprofile -service travis-ci" exited with 1.
Done. Your build exited with 1.

I enabled tests and removed one that was failing. Running some of them is still better than not running anything ;)

JurrianFahner commented 4 years ago

I don't understand why the test is failing, it works on my computer (after pulling the latest changes). There is no setup needed, I think.

@ojagodzinski Do you understand what is happening, based on your experience?

ojagodzinski commented 4 years ago

I don't understand why the test is failing, it works on my computer (after pulling the latest changes). There is no setup needed, I think.

There is a problem with len(id.String() > 36 condition, test cases have much shorter ids

JurrianFahner commented 4 years ago

I don't understand why the test is failing, it works on my computer (after pulling the latest changes). There is no setup needed, I think.

There is a problem with len(id.String() > 36 condition, test cases have much shorter ids

That explains a lot indeed! The reason I added the len(id.String() > 36 is to add some safety to avoid problems when an ID is not resolved and becomes an empty string (it would match, which is not desirable). Before my change it wouldn't be a problem, since the method matched on equality. Now we are matching on the start of the strings, because the last part can't be predicted. I've chosen for 36, because it is the length of an uuid (which is an arbritary choice).

In my opinion we have two solutions:

  1. Refactor all other tests for ids, to make sure that each id has more than 36 characters. But that is only reasonable when we are sure there is no use-case for this software where the ids are smaller. This might be also the safest option.

  2. Lower the length of the minimal id in a way that all tests would succeed, which is definitely faster to change and least invasive. But might add some insecurity, when the id of an event might match more than one task.

@ojagodzinski What would you like me to do? I'll pick up the task on monday.

ojagodzinski commented 4 years ago

Refactor all other tests for ids, to make sure that each id has more than 36 characters. But that is only reasonable when we are sure there is no use-case for this software where the ids are smaller.

It is the most dangerous one. We cannot force apps to use names with some arbitrary length.

is to add some safety to avoid problems when an ID is not resolved and becomes an empty string

so it should fail if ID is not resolved.

Lower the length of the minimal id in a way that all tests would succeed, which is definitely faster to change and least invasive. But might add some insecurity, when the id of an event might match more than one task.

divide this case into two separate ones and check id via regex not via HasPrefix in marathon >1.9

There should be also test case with two apps of lenght =1 that should not match in this condition.

JurrianFahner commented 4 years ago

I changed the code a bit, to make it possible to match on ids which are of length 1. Also the case when there is a search on an empty string is covered, by the code.

Downside of this approach that for each search the whole array of tasks needs to be searched, to add more safety. Before this change the search was stopped when a task was found.

JurrianFahner commented 4 years ago

make release check works on my machine. I don't know why it fails on travis, since I can't reproduce the failed build.

@ojagodzinski What can I do to solve that problem?

JurrianFahner commented 4 years ago

@ojagodzinski How can we resolve the issues in the pull request? I can't reproduce the issues on the travis build.