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

Fall back to nerdvegas Windows Docker Image #830

Merged

Conversation

j0yu
Copy link
Contributor

@j0yu j0yu commented Jan 8, 2020

Fixes Windows CI tests failing on forked repositories where ${gh_user}, e.g. wwfxuk, does not have a DockerHub image available.

For example, from #749 for commit cee7216

Here is this branch's Windows CI on:

@nerdvegas
Copy link
Contributor

Cheers for the fix!

I do wonder if the image fallback is needless complication though. Would anyone else really want to be hosting their own rez Windows CI docker image (and if they did, they could just change their own workflow anyway)?

For the sake of simplicity, should we not just replace ${gh_user} with nerdvegas?

@j0yu
Copy link
Contributor Author

j0yu commented Jan 9, 2020

Would anyone else really want to be hosting their own rez Windows CI docker image (and if they did, they could just change their own workflow anyway)?

I guess so, it would probably make sense if they're PRing the Windows Dockerfiles and want to be able to create images themselves.

There's also mention of moving to use GitHub Docker Packages for these images too in #789 which might be better to keep things within the GitHub ecosystem.

For the sake of simplicity, should we not just replace ${gh_user} with nerdvegas?

Done! Also simplified to just the run command as it will should automatically pull from DockerHub

@nerdvegas nerdvegas merged commit 51a5e3a into AcademySoftwareFoundation:master Jan 15, 2020
@j0yu j0yu deleted the testing-windows-ci-docker branch January 15, 2020 14:41
@bfloch
Copy link
Contributor

bfloch commented Jan 16, 2020

This was just useful during development of the image, but it can be easily controlled in forks etc.

Thanks for the fix!

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

Successfully merging this pull request may close these issues.

3 participants