Skip to content

Conversation

dduportal
Copy link
Contributor

@dduportal dduportal commented Aug 4, 2025

Ref. jenkins-infra/helpdesk#4557

This PR is focused on the Windows test harnesses and reverts #531 along with a few changes to allow these tests to be run without failures.

The main change is to remove the embedded ssh.exe and its dll binary file and instead rely on the ssh.exe in the environment where the tests are running. These binary where introduced 5 years ago in #49:

  • No update since then (so the ssh.exe is not really up to date and refuses the ciphers set in the SSH server of the SUT)
  • The Windows VM agents did not had OpenSSH back in time. But it changed 3 years ago so let's remove the workaround

A few cleanups are also added:

  • Removed all the "sub steps" added to debug the problem in march/April 2025 and that we did clean since then (checking list of containers, getting logs of container, etc.)
  • Removed all of the Start-Sleep -Seconds 10 instructions: unneeded anymore

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal dduportal force-pushed the revert-531-revert-496-revert-495-helpesk4557-disable-failing-test-for-now branch from ea1209b to bbdce5f Compare August 4, 2025 08:32
@dduportal dduportal added the tests label Aug 4, 2025
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal dduportal marked this pull request as ready for review August 4, 2025 09:48
@dduportal dduportal requested a review from a team as a code owner August 4, 2025 09:48
@dduportal dduportal merged commit afa9d48 into master Aug 4, 2025
11 checks passed
@dduportal dduportal deleted the revert-531-revert-496-revert-495-helpesk4557-disable-failing-test-for-now branch August 4, 2025 10:50
Comment on lines 159 to 166
It 'can check running containers' {
$exitCode, $stdout, $stderr = Run-Program 'docker' "container ls"
$exitCode | Should -Be 0
}

It 'can get logs of running container' {
$exitCode, $stdout, $stderr = Run-Program 'docker' "logs `"$global:CONTAINERNAME`""
$exitCode | Should -Be 0
Copy link
Contributor

Choose a reason for hiding this comment

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

@dduportal: Was removing those tests intentional? Looking at the commit message, it doesn't seem so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, these changes don't appear in https://github.com/jenkinsci/docker-ssh-agent/pull/538/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was intentional as per the PR description:

Removed all the "sub steps" added to debug the problem in march/April 2025 and that we did clean since then (checking list of containers, getting logs of container, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(note: these 2 blocks were not test in any way, or you have to explain what were they testing ;) )

Copy link
Contributor

@lemeurherveCB lemeurherveCB Aug 4, 2025

Choose a reason for hiding this comment

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

I was a bit confused when reviewing your PR commit per commit without taking too much attention to the PR body. My bad, make perfect sense now, thanks for the explanations!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the careful review! 👍

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

Successfully merging this pull request may close these issues.

3 participants