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.

Sign up for GitHub

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

Jump to bottom

Windows Tests via Docker #781

Merged
merged 6 commits into from
Nov 6, 2019
Merged

Windows Tests via Docker #781

merged 6 commits into from
Nov 6, 2019

Conversation

bfloch
Copy link
Contributor

@bfloch bfloch commented Oct 30, 2019 •
edited
Loading

This PR includes two fixes for cmd/powershell on windows and requires #775 to succeed future Windows tests.

Currently relying on hub.docker.com as the github service is broken:
https://gh.neting.ccmunity/t5/GitHub-Actions/Docker-Push-to-Package-Registry-from-Windows/m-p/36393#M2560

Setup Before Merging
(as configured in the PR, feel free to change if needed)

  1. Create a hub.docker.com account matching the github name: nerdvegas.
  2. Create an empty repository called rez-test-win
  3. In hub.docker.com go to Account/Security/New Access Token
    Choose a name (like "Github Access Token") and copy the token
  4. In github, repo settings create a secret DOCKERHUB_TOKEN and paste the TOKEN from docker

Once merged the Compose Windows Docker Image action should run.
The Windows Docker tests will fail at this point, but future pushes should succeed.

In future any required change to the docker image should be a separate PR as documented.
As such the regular actions will not run when only the docker image related files have been touched.
Similarly the image will not be rebuild when none of its files have been touched. Since the image is tagged with the latest revision on these files everything should always be in sync and use the correct images moving forward.

There is some ugliness in the duplication of code throughout the workflow files. GH Actions pros may find better ways to pass the environments instead of recalculating them.

Fingers crossed!

bfloch added 2 commits October 30, 2019 16:57
@bfloch
Fixing PowerShell to conform to other shells.
b277d26 
Initially adcac21 added rez to the
systempaths, but this should not be necessary.
It creates failure in tests if rez is not in the systempath, which should
not be a requirement for any shell.

Also conformed the parameters in cmd's and powershell's spawn_shell that
otherwise will end up in Popen_args.
@bfloch
Fix for test_rez_env_output failure in cmd-shell.
4bf62bf 
Fresh Windows systems have unexpanded %systemroot% in their PATH variable,
which causes `which()` to fail.
@bfloch
Copy link
Contributor Author

bfloch commented Oct 30, 2019 •
edited
Loading

Quick note: I left cmake out of the docker image.
We could enable it for the future, it was just pretty slow to install during my countless tests.
EDIT: Added it back.

@bfloch bfloch mentioned this pull request Oct 30, 2019
Closed
instinct-vfx
instinct-vfx approved these changes Oct 31, 2019
View reviewed changes
Copy link
Contributor

@instinct-vfx instinct-vfx left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seems good to me, still voting to move python into the docker image. But i would not want to let this delay this effort.

@bfloch
Copy link
Contributor Author

bfloch commented Oct 31, 2019

Thanks for the feedback.
I will also enable the cmake install in the Docker image. We're not using it but it's good we have it once we work on the cmake support.

@bfloch
Copy link
Contributor Author

bfloch commented Oct 31, 2019

Please ignore the pushes for a while since I'm testing on this branch on my fork :-/ Sorry for the noise.

@bfloch bfloch force-pushed the feature/windows_action_docker branch from 6c63710 to ca28dfe Compare October 31, 2019 14:29
@bfloch
Copy link
Contributor Author

bfloch commented Oct 31, 2019

Another thing I should check: If the image building and usage is in the same workflow I could express the dependency and we would not need to separate the process into two commits. I'll check if this is possible with github actions. Essentially I would need the 'paths:' checks within a conditional of a step.

@bfloch bfloch force-pushed the feature/windows_action_docker branch 16 times, most recently from 3a7510d to f195429 Compare October 31, 2019 18:07
@bfloch bfloch mentioned this pull request Oct 31, 2019
Open
@bfloch bfloch force-pushed the feature/windows_action_docker branch 3 times, most recently from f7dabea to 040c9ad Compare November 1, 2019 22:29
@bfloch bfloch force-pushed the feature/windows_action_docker branch 10 times, most recently from 735c820 to 7ee59e6 Compare November 4, 2019 16:19
bfloch added 3 commits November 4, 2019 11:52
@bfloch
Add docker based windows tests.
21b28cc 
Tests with regular github actions failed (potentially due to overflown PATH
variable), so instead this tries to test via a clean windows docker image.
@bfloch
Do not pull to check (uses experimental docker cli)
2fd5848
@bfloch
Optimizations and security improvements
0666152
@bfloch bfloch force-pushed the feature/windows_action_docker branch from 73c6894 to 9f83ddc Compare November 4, 2019 16:53
@bfloch
Copy link
Contributor Author

bfloch commented Nov 4, 2019

History cleaned up. Verified it working. The PR is open again.

@bfloch bfloch force-pushed the feature/windows_action_docker branch from 9f83ddc to 49f89bb Compare November 4, 2019 20:34
JeanChristopheMorinPerso
JeanChristopheMorinPerso approved these changes Nov 5, 2019
View reviewed changes
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

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

The docker side looks good to me. Big thanks your work on this :)

@nerdvegas nerdvegas merged commit 49f89bb into AcademySoftwareFoundation:master Nov 6, 2019
@bfloch bfloch mentioned this pull request Nov 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeanChristopheMorinPerso JeanChristopheMorinPerso JeanChristopheMorinPerso approved these changes

@nerdvegas nerdvegas nerdvegas approved these changes

@instinct-vfx instinct-vfx Awaiting requested review from instinct-vfx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bfloch @nerdvegas @instinct-vfx @JeanChristopheMorinPerso