[manila]: introduce WaitForStatus helpers for manila Shares, Replicas…

[kayrus]

Resolves #2652

[coveralls]

coverage: 78.94% (-0.1%) from 79.05% when pulling 2abd0b8 on kayrus:manila-wait into efb556a on gophercloud:master.

[kayrus]

cc @mandre @pierreprinetti kindly ping.

[pierreprinetti]

Thank you for your addition. I finally find a moment to look into it.

I appreciate the need for polling logic in a consumer application. However, I wonder if Gophercloud is the right place where to code it.

The WaitFor function doesn't do anything OpenStack-specific and I honestly would like to deprecate and remove it from Gophercloud at some point. A function like that should accept a context.Context instead, and there are a gazillion libraries out there that do that job.

So my question is: would this addition solve a problem that you can't reasonably solve using a retry function from a specialised library?

[kayrus]

@pierreprinetti I was under impression that once WaitForStatus functions are already in the codebase, then it makes sense to add them to another packages. I understand that WaitFor doesn't do anything openstack-related, but gophercloud is an SDK and helps people to build their applications in a more convenient and clean way, and I'd not remove this functionality from gophercloud.

There was another initiative, when IDFromName were moved to gophercloud/utils. I cannot say that it really helped, from my point of view it just added more confusion. But if you think that these utils have to be removed from gophercloud, I'd just moved them to utils then...

[pierreprinetti]

I think I understand your point of view. You are perfectly right: other packages use the same WaitFor function and they have been since forever. Your addition looks consistent with Gophercloud as it is.

My point is: if given the choice, I'd rather not maintain a retry library in this repository. utils may have been sort of a compromise. However if it were for me, I'd consider removing WaitFor in Gophercloud v2. I understand that consumer applications need that logic; on the other hand, they can find equivalent (or better) code somewhere else, or implement it on their own. I can imagine that based on the application you write, or based on the cloud you operate with, you will want a different polling interval, or you may want to be able to cancel operation if your caller is not interested in the result any more (e.g. when your application acts upon an HTTP-induced trigger and the connection is closed). You may even want exponential backoff in some situations. That's a whole bunch of use cases we're unlikely to satisfy, and for the reason that it's not really Gophercloud's business. That's why I would immediately deprecate WaitFor and plan its removal.

So my question is: why do you think it's best to have this function here, rather than directly in you application's code?

Also note that I am not the sole decision maker here! Let's see what others think @mandre @EmilienM

[kayrus]

why do you think it's best to have this function here, rather than directly in you application's code?

I work with a number of OpenStack consumer projects that use these shared functions. Moving them into each project means: more code duplication, complicated maintainability, potential license copyright problems, less attention from the community for code improvement.

I'd rather move them to utils. If necessary, the context support can be added latyer.

[pierreprinetti]

I'd rather move them to utils.

Let's see what the other Gophercloud maintainers think. I don't think your function should go into utils: either we keep consistent and accept them here in gophercloud/gophercloud, or we deprecate the wait functions altogether. After a quick search on Github, I didn't really find that gazillion alternative modules I had imagined.

[mandre]

I personally don't see a problem having convenience functions in gophercloud to make it easier to consume. Let's also remember we are also consumers, via the acceptance tests, although we're using a different implementation of WaitFor() 😅
The other one being in util.go.

[EmilienM]

I'll like to see this function in https://github.com/gophercloud/gophercloud/blob/master/util.go if possible and use it from acceptance, so we can remove

gophercloud/acceptance/tools/tools.go

Lines 15 to 37 in 157270b

	// WaitFor uses WaitForTimeout to poll a predicate function once per second to
	// wait for a certain state to arrive, with a default timeout of 300 seconds.
	func WaitFor(predicate func() (bool, error)) error {
	return WaitForTimeout(predicate, 600*time.Second)
	}
	// WaitForTimeout polls a predicate function once per second to wait for a
	// certain state to arrive, or until the given timeout is reached.
	func WaitForTimeout(predicate func() (bool, error), timeout time.Duration) error {
	startTime := time.Now()
	for time.Since(startTime) < timeout {
	time.Sleep(2 * time.Second)
	satisfied, err := predicate()
	if err != nil {
	return err
	}
	if satisfied {
	return nil
	}
	}
	return ErrTimeout
	}

at some point.

[kayrus]

I can also implement something that uses context.