-
Notifications
You must be signed in to change notification settings - Fork 407
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
Features/windows agents with pathing issues fixed #184
Features/windows agents with pathing issues fixed #184
Conversation
- Fixing haphazard changes to Docker.groovy - Fixing FindBugs issue with annotations on WindowsDockerClient.launch()
@dwnusbaum perhaps it would be best to setup import order and codestyle to avoid fighting over imports 😆 |
@Casz sure, if you want to open a PR to enforce some kind of import order, fine by me. |
As someone who could REALLY USE Windows Docker agents, I'm extremely excited about this pull request. I've been wanting to integrate Windows docker agents for over a year now. However, I got so frustrated by the lack of interest from the maintainers of this plugin that I gave up waiting and started using bat commands to use it. @Casz, I really home you can get this PR accepted. |
I've just run into the limitation of this plugin that it doesn't support windows and am glad to see a PR ready to fix it. Is there anything else needed before this can be merged? This would make a lot of things easier for me. |
argb.add("-w", workdir); | ||
} | ||
for (Map.Entry<String, String> volume : volumes.entrySet()) { | ||
argb.add("-v", volume.getKey() + ":" + volume.getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use --mount
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, I notice the regular docker client uses volumes instead of mounts as well. Should probably keep them consistent (which I suspect means switching DockerClient.java to mounts too mind you - and a separate PR)
I'll properly end up spending the weekend fixing this up 😓 |
@@ -75,11 +75,17 @@ class Docker implements Serializable { | |||
new Image(this, id) | |||
} | |||
|
|||
String shell() { | |||
node { | |||
script.isUnix() ? "sh" : "bat" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to use powershell
as the default for Windows based containers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
powershell or do you want pwsh? 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it looks like the powershell
step currently only supports powershell.exe
not pwsh.exe
, so sticking with bat
is the best option.
Big +1 from me on this one. |
@slide I hope we can work on getting this tested in Jenkins CI :) |
@Casz what do you need for that? We have a label 'windock' for windows docker agents. |
Neat! All I would need, so good to know 😍 |
Looks like several of the tests need to be updated to run only on Linux systems. |
Click the button! We needses it! 😄 |
@Casz I know this might be a big ask, but is there a way you could write up some docs on how to use this branch in an instance of jenkins? Maybe you should just fork this repo and publish this plugin? I would certainly use it. You can call it the |
I was actually able to get this repo forked, this PR merged, project build, and hpi file installed in jenkins in under a few hours. So not too bad at all. I can confirm...I have it working. @Casz Not sure if it is related to this PR but I have to specify my registry instead of it assuming the global registry. So it inspects dockerhub first then tries my local repo. Not really a big deal though since it does try the globally referenced repo in the end. Again I doubt this is related, just wanted to mention it in case you it triggered something for you. Great work! |
I am about releasing it if there is a consensus. |
@rbutcher Thanks for doing all the heavy lifting! I just fixed up the path issues! @oleg-nenashev thanks for making the release. Release notes are available here https://github.com/jenkinsci/docker-workflow-plugin/releases/tag/docker-workflow-1.21 Thanks for all the work to enable release drafter and thanks @toolmantim just an all around thank you 😊 |
This is so great, thanks guys, it works! One feature request though, could you also make it work with |
@gegles Please file an issue in Jira at https://issues.jenkins.io. Adding additional comments to this PR is not really a good place to request features. |
@gegles it should already work. Labeling agents becomes very important :) I build this pipeline last week and been working great: #! /usr/bin/env groovy
pipeline {
agent {
dockerfile {
filename "Dockerfile.CLI"
label "windows"
}
}
stages {
stage('Nuget restore') {
steps {
powershell "dotnet restore --source https://artifactory.company.com/api/nuget/v3/nuget"
}
}
stage('Build solution') {
steps {
powershell "dotnet build -c:Release --no-restore -v:m"
}
post {
always {
// MSBuild warnings publisher
recordIssues tools: [msBuild()], enabledForFailure: true, qualityGates: [
[threshold: 1, type: 'TOTAL', unstable: false]
]
}
}
}
stage('Pack') {
steps {
powershell 'remove-item ./publish/ -Force -Recurse -ErrorAction SilentlyContinue'
powershell "dotnet publish projectCLI/projectCLI.csproj -c:Release -v n --no-restore --output publish"
zip dir: 'publish', zipFile: 'publish/project.zip'
archiveArtifacts 'publish/*.zip'
}//steps
}//stage
}//stages
}//pipeline |
@Casz Thanks for your response. I tried again and here is what I see. When I try using the following (with a pre-built image), it works: stage('Windows') {
agent {
docker {
image 'my-win-builder'
label 'windows && docker && x86_64'
}
} When I try with the following: stage('Windows') {
agent {
dockerfile {
dir 'docker\\build\\windows'
label 'windows && docker && x86_64'
}
} It fails with the classic Cannot run program "nohup" error. Same issue if I move and rename my windows Dockerfile to the top-level dir: stage('Windows') {
agent {
dockerfile {
filename 'Dockerfile.Windows'
label 'windows && docker && x86_64'
}
} Any thoughts? |
oh it works for us because someone installed git with linux tools in our jenkins agent image 😓
I'll look into improving dockerfile then :) |
I see. Glad you figured out the root cause. Yes, much appreciated if you can sort this out! I am not knowledgeable enough about Jenkins internals to do it myself... Let me know if there is a specific ticket to track this. Cheers. G. |
Ok, looking for a workaround, I've tried reinstalling git on our Windows agent the same way you did (and rebooted), but I still get the same error... How do you guys connect Jenkins to the slave? we use ssh (even on windows). |
@Casz Thank you very much for your efforts on this feature. Is it possible to use Windows containers also with the scripted pipeline syntax? |
@albertosottile should be the same. @gegles we use JNLP |
Ok, after some more trial and error, I got the workaround working by setting the DefaultShell for OpenSSH to the git bash shell: New-ItemProperty -Path "HKLM:\SOFTWARE\OpenSSH" -Name DefaultShell -Value "C:\Program Files\Git\bin\bash.exe" -PropertyType String -Force This will get us going for now. Thanks for all your work and for sharing. |
Thanks all. Would be great to get it documented for other adopters, better
just in Github README for now
…On Thu, Oct 10, 2019, 08:35 Guillaume Egles ***@***.***> wrote:
Ok, after some more trial and error, I got the workaround working by
setting the DefaultShell for OpenSSH to the git bash shell:
New-ItemProperty -Path "HKLM:\SOFTWARE\OpenSSH" -Name DefaultShell -Value "C:\Program Files\Git\bin\bash.exe" -PropertyType String -Force
This will get us going for now. Thanks for all your work and for sharing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#184?email_source=notifications&email_token=AAW4RIEA4ERANBBLEXKNMOLQN3EMPA5CNFSM4IO6OVRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA27FOI#issuecomment-540406457>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAW4RIFXSTREXGABBOUYYNDQN3EMPANCNFSM4IO6OVRA>
.
|
Someone should tweet that this has been released, maybe Jenkins or cloudbees, we want to retweet. |
@solvingj Oleg already did: https://twitter.com/oleg_nenashev/status/1181991792352337926 😄 |
@Casz Thanks for taking this crusade over for me. After the chat over on my PR I thought this was dead in the water and have only been passively monitoring the progress of this. I was surprised when a buddy of mine at work told me this got merged yesterday. Crazy happy to see this merged. Thanks folks! |
We ran into this as well. I had completely forgotten about it because I solved it while I was starting to refactor and rebuild the plugin myself before finding this PR. I solved it via the following. Although this method is less ideal because it requires manual GUI steps. Powershell or other CLI methods are preferred for lots of reasons.
|
I don't want workarounds 😄 I want actual fixes and the PR is already up: jenkinsci/pipeline-model-definition-plugin#354 |
Excellent! That's the right way for sure! |
Ran into some situations where we might be forced to use |
@gegles should be solved in v1.4.0 of pipeline-model-definition |
It seems tobe working!! Thank you! |
Hi, so I'm trying to use it with latest version 1.23 of workflow plugin, and even with the simplest example of just running the container it fails to recognize that it is a windows not linux container: Reason: Template provisioning failed.
jenkins script:
any thoughts? please help. |
@penszo please see the above comment above as there is a fix to that issue: |
@jetersen thanks for super fast response. Regarding your comments - I'm using the latest 1.6.0 version of this plugin. Still doesn't work for me :( and this is such a nice feature. I would love to have it up and running. |
@penszo I honestly only tested this using Windows Server 2019 since Microsoft recommends that for any docker workload |
@penszo Did you find a solution for this problem ? I'm struggling with the same problem on windows docker. Jenkins report:
|
resolve #148
resolve #183
This is what we had issues with b0d3fae when testing out the branch made by @rbutcher
We have been running this in production since November/December of last year 😓
Allows us to use a declarative pipeline like
For a windows docker host
For a linux docker host