-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add docker on windows integration tests on github actions #6580
Conversation
Hi @alonyb. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alonyb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Can one of the admins verify this patch? |
Codecov Report
@@ Coverage Diff @@
## master #6580 +/- ##
==========================================
+ Coverage 37.19% 37.86% +0.67%
==========================================
Files 146 147 +1
Lines 9106 9089 -17
==========================================
+ Hits 3387 3442 +55
+ Misses 5303 5222 -81
- Partials 416 425 +9
|
I personally applaud the hard work you put in making this work ! thank you |
@alonyb I know this is WIP , but one early comment , the name of the job should be in form of and I wonder if the test needs to run with power user privileges or not... |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
I fixed that. |
W/remove makefile changes
@blueelvis any chance you could help with this ? can we run docker driver test on windows in github actions? |
Do you mind resolving the merge conflict? |
Resolved |
This seems to be working now, any objections to merging? |
@@ -88,7 +88,7 @@ func status() registry.State { | |||
// Allow no more than 2 seconds for querying state | |||
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | |||
defer cancel() | |||
|
|||
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.
Revert unrelated changes
Makefile
Outdated
@@ -280,22 +280,45 @@ ifeq ($(MINIKUBE_BUILD_IN_DOCKER),y) | |||
$(call DOCKER,$(BUILD_IMAGE),/usr/bin/make $@) | |||
endif | |||
which go-bindata || GO111MODULE=off GOBIN="$(GOPATH)$(DIRSEP)bin" go get github.com/jteeuwen/go-bindata/... | |||
ifeq ($(OS),Windows_NT) |
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.
Was this to make golint happy ? I rather this be not added to makefile.
Could be changed in a different place
run: | | ||
curl -LO https://github.com/medyagh/gopogh/releases/download/v0.1.16/gopogh.exe | ||
shell: bash | ||
- name: Download binaries |
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.
I would add a step here to make sure docker is installed and is healthy
maybe this
- name: install or check docer
run: |
.... commands to install docker...
docker info || true
docker ps || true
...
Makefile
Outdated
pkg/minikube/translate/translations.go: $(shell find "translations/" -type f) | ||
ifeq ($(MINIKUBE_BUILD_IN_DOCKER),y) | ||
$(call DOCKER,$(BUILD_IMAGE),/usr/bin/make $@) | ||
endif | ||
ifeq ($(OS),Windows_NT) |
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.
it is not clean to me why you had to modify the makefile to make this work ? is there any way to avoid this hack ?
name: minikube_binaries | ||
- name: run integration test | ||
continue-on-error: true | ||
run: | |
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.
Pass the SHELL environment variable to this test like other one. So it passes the docker-env.
Also maybe rename the test to Docker_windows_shell
So in the future we can add another test called
Docker_windows_powershell
numFail=$(echo $STAT | jq '.NumberOfFail') | ||
echo "----------------${numFail} Failures----------------------------" | ||
echo $STAT | jq '.FailedTests' || true | ||
echo "--------------------------------------------" |
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.
since this PR we added ***Pass
please copy that part from other tests so it inludes the Passes Num
runs-on: windows-latest | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- name: Download gopogh |
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.
Add a step to show docker version
and docker info
like other tests
@alonyb looking at the results it appears that it is using hyperv driver
|
I know it's partially broken right now, but I'm going to merge this as it's super important for us to iterate towards. Thank you for all of your hard work! |
This PR will add integration tests for windows.