Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Add --git-timeout configuration flags #1416

Merged
merged 5 commits into from
Oct 4, 2018

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Oct 1, 2018

Closes #1414

This PR adds a --git-time-out --git-timeout flag to both fluxd and the helm operator.

I was unsure what to do with the gitOpTimeout constant in daemon/loop.go:25. If someone could give me guidance in this I am more than happy to make additional changes.

Resulting builds from changes in this PR can be pulled and tested from:
hiddeco/flux:1414-git-timeout-9dc6b2181744
hiddeco/helm-operator:1414-git-timeout-9dc6b2181744

@sfrique
Copy link
Contributor

sfrique commented Oct 1, 2018

I don't know who takes care of the helm chart, but it would be nice to add the option there as well.
the current values -> https://github.com/weaveworks/flux/blob/master/chart/flux/values.yaml has quite a few options.

I tested and it fixed the problem I was having, thanks a lot.


// Chart releases sync due to Custom Resources changes -------------------------------
{
mainLogger.Log("info", "Attempting to clone repo ...", "url", gitRemote.URL)
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
ctx, cancel := context.WithCancel(context.Background())

This comment was marked as abuse.

This comment was marked as abuse.

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reservations over using the same timeout for all git operations -- increasing the timeout for git clone might mean that other operations take much longer to fail. But I don't know that's a problem, and it can be addressed if it turns out to be. Meanwhile, this is worth it just for removing all the ad-hoc constants :-)

One request, as commented.

@@ -93,6 +93,7 @@ func main() {
gitSkipMessage = fs.String("git-ci-skip-message", "", "additional text for commit messages, useful for skipping builds in CI. Use this to supply specific text, or set --git-ci-skip")

gitPollInterval = fs.Duration("git-poll-interval", 5*time.Minute, "period at which to poll git repo for new commits")
gitTimeOut = fs.Duration("git-time-out", 20*time.Second, "duration after which git operations time out")

This comment was marked as abuse.

This comment was marked as abuse.

@hiddeco hiddeco changed the title Add --git-time-out configuration flags Add --git-timeout configuration flags Oct 4, 2018
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grand, thanks Hidde 🐝

@@ -190,7 +190,7 @@ The following tables lists the configurable parameters of the Weave Flux chart a
| `git.label` | Label to keep track of sync progress, used to tag the Git branch | `flux-sync`
| `git.ciSkip` | Append "[ci skip]" to commit messages so that CI will skip builds | `false`
| `git.pollInterval` | Period at which to poll git repo for new commits | `5m`
| `git.timeOut` | Duration after which git operations time out | `20s`
| `git.timeout` | Duration after which git operations timeout | `20s`

This comment was marked as abuse.

This comment was marked as abuse.

@hiddeco hiddeco merged commit ebc78e3 into fluxcd:master Oct 4, 2018
@hiddeco hiddeco deleted the 1414-git-timeout branch October 4, 2018 15:34
@latchmihay
Copy link

the gitOpTimeout constant in daemon/loop.go:25 which is currently 15s gets hit on slower/remote networks, and it waits for entire loop cycle until its retried.

Is it possible for it to use--git-timeout argument for gitOpTimeout constant. I understand the concerns that it will slow everything down to timeout, but it should be fine to whoever is changing it.

Having the --git-timeout default of 20s to gitOpTimeout constant shouldn't be that big of a deal since its additional 5s

@hiddeco
Copy link
Member Author

hiddeco commented Feb 25, 2019

@latchmihay your reasoning makes sense to me and the additional 5s probably does not hurt anyone.

I will turn it into a PR tomorrow and see what others think.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants