Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `--timeout` flag to `fluxctl` #2056

Merged
merged 3 commits into from Aug 20, 2019

Conversation

@suvl
Copy link
Contributor

commented May 16, 2019

In our cluster, there's a few operations that almost always take more than 60 seconds. sync is the most sensitive for us, as we use it from tools like Jenkins and Azure DevOps to assert that our changes were applied to the cluster.

The hard-coded timeout prevented us from having a nice deterministic pipeline, so we need to change it: we added the --timeout <int> argument and it'll also be looking to the FLUX_TIMEOUT env variable.

.gitignore Outdated Show resolved Hide resolved
cmd/fluxctl/root_cmd.go Outdated Show resolved Hide resolved
@squaremo

This comment has been minimized.

Copy link
Member

commented May 16, 2019

This is a nice idea 👍

Is there an advantage to having the timeout in fluxctl, rather than using e.g., timeout 2m fluxctl sync?

I can think of one: it'll be available in environments which don't have GNU coreutils, e.g., Windows -- and since you mentioned Azure, perhaps that's an important advantage for you :-)

@suvl

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@squaremo well, doin' it with timeout is a quite nice idea, but since the timeout is currently hard-coded, and as you said it can be advantageous in some cases, I still would like to see a setting/env var on this.

@hiddeco

This comment has been minimized.

Copy link
Member

commented May 16, 2019

This PR will probably solve #1857.

Copy link
Member

left a comment

Given the issue I mentioned I think this is a good change and will even solve some edge case scenarios people are having issues with at the moment.

Please take a look at my comments (all about code style and not so much about the contents of the PR itself).

@suvl

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@hiddeco I've accepted all proposed changes.

@squaremo

This comment has been minimized.

Copy link
Member

commented May 20, 2019

This PR will probably solve #1857.

That would be nice, but I don't think it will happen this time -- this PR changes how long fluxctl will keep polling for a job to complete, but the job timeout internal to fluxd remains the same.

@squaremo

This comment has been minimized.

Copy link
Member

commented May 20, 2019

I still would like to see a setting/env var on this.

Fair enough -- no objection here.

@hiddeco

This comment has been minimized.

Copy link
Member

commented May 20, 2019

That would be nice, but I don't think it will happen this time -- this PR changes how long fluxctl will keep polling for a job to complete, but the job timeout internal to fluxd remains the same.

Ah, you are right 😞


@suvl looking good, tests are failing due to the introduced timeout var not being set in TestReleaseCommand_CLIConversion. You can run the tests on your own machine by running make test (to confirm you fixed the issue), in addition, can you smash your commits together once this has been fixed?

@hiddeco hiddeco added this to the v1.13.0 milestone May 22, 2019
@2opremio

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

@suvl are you planning to continue working on this? If that's the case, please revise on master since we have recently moved to go modules, which is causing a conflict

@suvl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@2opremio yeah sorry I was away for a time, but yeah planning on fixing the tests and now on fixing the merge from master.

@squaremo

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

fixing the merge from master.

Please rebase on master rather than merging. Usually there will be no conflicts. If you need a hand though, let us know.

EDIT: After re-reading the thread, I think that's what you meant. As you were :-)

@squaremo squaremo modified the milestones: v1.13.0, 1.13.1, 1.14.0 Jun 13, 2019
@suvl suvl changed the title Add timeout setting [WIP] Add timeout setting Jun 18, 2019
@suvl suvl force-pushed the suvl:feature-timeout branch from 04f161f to d224c25 Jun 18, 2019
@hiddeco hiddeco force-pushed the suvl:feature-timeout branch 2 times, most recently from 1d18f96 to 18768fc Jul 11, 2019
@hiddeco hiddeco changed the title [WIP] Add timeout setting Add `--timeout` flag to `fluxctl` Jul 11, 2019
@hiddeco hiddeco requested review from squaremo and stefanprodan Aug 15, 2019
@hiddeco

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Can't review this myself again as I have touched the code, @stefanprodan / @squaremo PTAL.

@hiddeco hiddeco force-pushed the suvl:feature-timeout branch from 18768fc to c2c8d0a Aug 19, 2019
Copy link
Member

left a comment

LGTM

@stefanprodan stefanprodan force-pushed the suvl:feature-timeout branch from c2c8d0a to a7b123e Aug 20, 2019
@stefanprodan stefanprodan merged commit 9a0977c into fluxcd:master Aug 20, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: helm Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.