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

Issue 731: Add new docker template option for container stop timeout #732

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

sithmein
Copy link
Contributor

Addresses #731 .

@sithmein
Copy link
Contributor Author

The test failures are very likely not caused by the PR since the same tests have also been failing for the latest commit on master.
I cannot run them locally, too, especially DockerComputerSSHConnectorTest always times out.

@pjdarton
Copy link
Member

Thanks for the PR.
I'm a bit snowed-under at work to deal with this right now (and going on vacation tomorrow for a week) so this isn't going to get merged immediately, but once I'm "back in the game" I'll take a detailed look and take it from there.
In the meantime, if you could "persuade" the JenkinsCI system to do a non-broken build, that would be good. FYI if you close the PR, wait 10-15 minutes (for the JenkinsCI system to notice it's gone), and then re-open the PR (i.e. re-open the old PR; don't create a new one) then the JenkinsCI system should have another try at building it.
If it's consistently borked, bring it to the attention of Jenkins folks on IRC e.g. http://webchat.freenode.net/?channels=jenkins-infra

Re: Can't run unit-tests locally
I used to have that problem too - until I updated docker on my test system. There's nothing special about the build environment that the JenkinsCI systems have so you should be able to run the build & tests locally.

@pjdarton
Copy link
Member

pjdarton commented Sep 5, 2019

I'm going to close this PR, wait a bit, and re-open it to try to get the Jenkins CI build to rebuild it - hopefully it'll build cleanly...

@pjdarton pjdarton closed this Sep 5, 2019
@pjdarton pjdarton reopened this Sep 5, 2019
Copy link
Member

@pjdarton pjdarton left a comment

Choose a reason for hiding this comment

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

This code would benefit from declaring a int constant of 10 to hold the default timeout, and then reference that everywhere.
If we can't manage to reference it "everywhere" then every declaration of 10 seconds needs a comment (aimed at any future maintainers of this code) saying where else it's defined and the importance of keeping them in step.
At present, we've got it being defaulted to 10 seconds in multiple places but 100 in another - we need to fix that and make that kind of mistake harder to make.

Aside from that, the code changes are nice and neat 😀

@pjdarton pjdarton added the enhancement A PR providing an enhancement to existing functionality. label Mar 11, 2020
@pjdarton pjdarton closed this Apr 6, 2020
@pjdarton pjdarton reopened this Apr 6, 2020
@pjdarton pjdarton merged commit e59f47a into jenkinsci:master Apr 7, 2020
@pjdarton pjdarton mentioned this pull request Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A PR providing an enhancement to existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants