Closed arschles closed 8 years ago
By analyzing the blame information on this pull request, we identified @jackfrancis to be a potential reviewer
From my naive surface-deep code review (not too well versed with workflow-manager... Need to do a deep dive one day), this all looks good to me.
Summary Of Issues
I opened this pull request to fix the panic described in #54. After fixing the panic described in that issue, I uncovered the following other bugs, in order of the below listing. Since each bug uncovers the next in turn, I believe that the fixes all need to be included into a single PR, even if this PR is large in scope.
time.Duration
to the pollertime.Duration(config.Spec.Polling)*time.Second*time.Second
intotime.NewTicker
, which was a negative number and causedtime.NewTicker
to return an errorVERSIONS_API_URL
andDOCTOR_API_URL
environment variables (among others) are not being set properly in the deis chartSummary of Fixes
This PR include the following fixes to address the above issues:
rest
package that has aClient
interface, whose implementation carries a shared*http.Client
and the base URL for the workflow-manager API server. As such, that data need not be passed everywhere the client is usedenvconfig
struct for theVERSIONS_API_URL
,DOCTOR_API_URL
, andAPI_VERSION
environment variables. These defaults allow this component to function until https://github.com/deis/charts/issues/267 is fixeddata
package to be non-exported. This change forces other packages to create these implementations via "constructor" functions, which ensures that all required fields are given. This design pattern forces all other packages to create new implementations in a single place, which greatly reduces the chances that other packages initialize an implementation withnil
members, and fixes #54types.Cluster
to(github.com/deis/workflow-manager/data).AvailableVersions
'sRefresh
func. With this change,AvailableVersions
no longer needs to callGetCluster
, which was the source of the recursiondata
package to no longer swallow errors. All code indata
should now check for and return errorsdata
package no longer logs errors. Instead, implementations just return the errors and rely on callers to handle logging, handling, etc...time.Duration
tojobs.DoPeriodic
and not multiplying thattime.Duration
bytime.Second
. Previously, the resultanttime.Duration
was an overflow andtime.NewTimer
was returning an errorjobs/jobs.go
Fixes https://github.com/deis/workflow-manager/issues/55 Fixes https://github.com/deis/workflow-manager/issues/54
Still TODO