create_unit should check fleet's response code instead of executing a get_unit call

[simonvanderveldt]

Currently create_unit executes a get_unit call after it has created the unit which makes create_unit fail sometimes with the following error message: Unable to create unit: unit does not exist (404)

Since fleet already returns a response code when handling units create_unit should simply check this response code and not call get_unit.

[simonvanderveldt]

@cnelson not sure if you'll see this, but I was wondering what the reason is you get the unit after creating it? Wouldn't it make more sense to just return nothing, since the fleet HTTP API returns a 201 with no response body?

If a consumer wants to get the just created unit he can easily call get_unit himself.

[cnelson]

While I agree with you that this does not match the fleet api directly, I'm hesitant to make a breaking change to the API as I have no way of knowing if people have written code that depends on create_unit returning the unit.

In what circumstances have you seen this fail?

[simonvanderveldt]

@cnelson thanks for the response! :)

While I agree with you that this does not match the fleet api directly, I'm hesitant to make a breaking change to the API as I have no way of knowing if people have written code that depends on create_unit returning the unit.

Yeah, I can understand that one. Maybe it should become a new major version?

In what circumstances have you seen this fail?

We make use of a small service wrapper on top of fleet built using this library. We do between 150-200 deployments a day of which about 25% run into this issue.
Strange thing is we actually starting having these issue once we lowered the load on our machines, we didn't have them before that.

[cnelson]

So would changing this to return True (or raising an exception on error) solve the problem for you?

[simonvanderveldt]

So would changing this to return True (or raising an exception on error) solve the problem for you?

@cnelson yeah, returning True is what I'm doing now, works perfectly.
It also gives the consumer the option to simply do if (fleet_client.create_unit('foo.service', unit) without the need for the consumer to do special comparisons.

P.S. The same goes for set_unit_desired_state()

[cnelson]

I will attempt to get this in over the holidays, or a PR would be appreciated :)

[simonvanderveldt]

I will attempt to get this in over the holidays, or a PR would be appreciated :)

👍
I'll be off for a couple of days, but if I can squeeze it in I'll create a PR :)