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

Revendor gardener/[email protected] #183

Merged
merged 4 commits into from
Oct 12, 2020

Conversation

prashanth26
Copy link

@prashanth26 prashanth26 commented Oct 11, 2020

How to categorize this PR?

/area delivery
/kind enhancement
/priority normal
/platform gcp

What this PR does / why we need it:
Revendors the latest gardener/gardener vendors.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Code changes are inspired by gardener/gardener-extension-provider-openstack#155. Credits: @kon-angelo

Release note:

`github.com/gardener/gardener` dependency is now updated to `v1.11.1`.

@prashanth26 prashanth26 requested review from a team as code owners October 11, 2020 19:26
@gardener-robot gardener-robot added needs/review Needs review kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else area/delivery Delivery related kind/enhancement Enhancement, improvement, extension platform/gcp Google cloud platform/infrastructure priority/normal labels Oct 11, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 11, 2020
@gardener-robot gardener-robot added the size/l Size of pull request is large (see gardener-robot robot/bots/size.py) label Oct 11, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 11, 2020
@prashanth26
Copy link
Author

Hi,

I have tried to revendor g/[email protected]. However, when i locally run make test i am facing the following issue with this PR

fatal error: checkptr: converted pointer straddles multiple allocations

goroutine 1 [running, locked to thread]:
runtime.throw(0x32501c5, 0x3a)
	/usr/local/opt/go/libexec/src/runtime/panic.go:1116 +0x72 fp=0xc0004986c8 sp=0xc000498698 pc=0x1079d72
runtime.checkptrAlignment(0xc0000abbf8, 0x310f7c0, 0x1)
	/usr/local/opt/go/libexec/src/runtime/checkptr.go:20 +0xc9 fp=0xc0004986f8 sp=0xc0004986c8 pc=0x1049569
github.com/karrick/godirwalk.readDirents(0xc0005b5fb0, 0x2b, 0xc0000ab000, 0x1000, 0x1000, 0x0, 0xc000000180, 0xc0004988c8, 0x10aeea8, 0xc000000180)
	/Users/i341888/Go/src/github.com/gardener/gardener-extension-provider-gcp/vendor/github.com/karrick/godirwalk/readdir_unix.go:51 +0x18c fp=0xc000498860 sp=0xc0004986f8 pc=0x2db9b6c
github.com/karrick/godirwalk.ReadDirents(...)

However, when I run the same without the -race option in the go test it works well. I tried to debug to find out the root cause, however, I was unable to find the root cause. Any ideas?

cc: @kon-angelo

@ialidzhikov
Copy link
Member

See gobuffalo/packr#261. For me it works fine using make test in docker container

$ docker run --rm -v ${GOPATH}/src:/go/src -it golang:1.15.2 bash
# cd src/github.com/gardener/gardener-extension-provider-gcp
# make test

@kon-angelo
Copy link
Contributor

That is exactly the issue I had with my local setup too.

@prashanth26 prashanth26 changed the title Revendor/[email protected] Revendor gardener/[email protected] Oct 11, 2020
@prashanth26
Copy link
Author

@ialidzhikov - I got a hint that it was an issue with my underlying setup. I tried upgrading my golang but didn't help. However, this issue explains it. Thanks for the hint. Saved my time 👍

@kon-angelo - Yes, I remember you vaguely mentioning this in the meeting and hence tagged you. Thank you as well 👍

kon-angelo
kon-angelo previously approved these changes Oct 11, 2020
Copy link
Contributor

@kon-angelo kon-angelo left a comment

Choose a reason for hiding this comment

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

/lgtm

Because our PRs are similar you could wait for a third opinion too :)

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review needs/second-opinion Needs second review by someone else labels Oct 11, 2020
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

Can you add "SKIP_FETCH_TOOLS=1" to make test?

@gardener-robot gardener-robot added needs/changes Needs (more) changes and removed reviewed/lgtm Has approval for merging labels Oct 11, 2020
@@ -127,7 +127,7 @@ format:

.PHONY: test
test:
@$(REPO_ROOT)/vendor/github.com/gardener/gardener/hack/test.sh ./cmd/... ./pkg/...
@SKIP_FETCH_TOOLS=1 $(REPO_ROOT)/vendor/github.com/gardener/gardener/hack/test.sh ./cmd/... ./pkg/...
Copy link
Member

Choose a reason for hiding this comment

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

Can you add it also to test-cov?

Copy link
Author

Choose a reason for hiding this comment

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

Object: expectedImages,
}
ctx := context.TODO()
c.EXPECT().Get(ctx, gomock.Any(), gomock.AssignableToTypeOf(&extensionsv1alpha1.Worker{})).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
c.EXPECT().Get(ctx, gomock.Any(), gomock.AssignableToTypeOf(&extensionsv1alpha1.Worker{})).
c.EXPECT().Get(ctx, gomock.Any(), gomock.AssignableToTypeOf(&extensionsv1alpha1.Worker{})).Return(nil)

Copy link
Author

Choose a reason for hiding this comment

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

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 12, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 12, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 12, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 12, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 12, 2020
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/delivery Delivery related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/gcp Google cloud platform/infrastructure size/l Size of pull request is large (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants