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

fix: use correct syntax for environment variable #244

Merged
merged 4 commits into from
Jan 28, 2022

Conversation

viceice
Copy link
Member

@viceice viceice commented Jan 19, 2022

Environment variables need an other syntax for be usable on windows bat.
This PR fixes JENKINS-67578 and JENKINS-67589.
Commit b3a6996 has broken this plugin for windows users.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@viceice
Copy link
Member Author

viceice commented Jan 19, 2022

@car-roll @rsandell Not sure how to test this

@viceice viceice marked this pull request as ready for review January 19, 2022 13:59
@car-roll
Copy link
Collaborator

You can try to replicate some of the examples in the Jenkins issues in the corresponding test class. Just looking quickly, I can see some examples using withServer and withRegistry. Those methods are tested in https://github.com/jenkinsci/docker-workflow-plugin/blob/master/src/test/java/org/jenkinsci/plugins/docker/workflow/DockerDSLTest.java
That might be the place to start.

@viceice
Copy link
Member Author

viceice commented Jan 20, 2022

Tests timed out 😕

@TBBle
Copy link
Contributor

TBBle commented Jan 21, 2022

Something has broken on the CI cluster in the last week, because steps before the code changes are taking 10's of minutes that previously took 10's of seconds.

Oh, since I last looked a new run happened and appears to have been back to the previous times. So the problem must have been temporary, or has been resolved.

@viceice
Copy link
Member Author

viceice commented Jan 27, 2022

@viceice
Copy link
Member Author

viceice commented Jan 27, 2022

Will try to add some tests now

@viceice
Copy link
Member Author

viceice commented Jan 27, 2022

#245

Seems not possible yet, as there is no docker infra on the ci windows agents

@rsandell
Copy link
Member

@viceice Thanks for the fix! If you can vouch that it works on your windows machine I think that would be good enough for me to merge :)

@viceice
Copy link
Member Author

viceice commented Jan 27, 2022

@viceice Thanks for the fix! If you can vouch that it works on your windows machine I think that would be good enough for me to merge :)

Will validate on our jenkins now

@viceice
Copy link
Member Author

viceice commented Jan 27, 2022

Verified, works for me with artifact from jenkins build of this PR. 🎉

@rsandell rsandell merged commit e20ab32 into jenkinsci:master Jan 28, 2022
@viceice viceice deleted the fix/windows-env-vars branch January 28, 2022 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants