Fix panic on pointer to embedded struct #173

christarazi · 2020-09-17T18:53:31Z

This PR converts an embedded struct into its original type if it is
a pointer.

Fixes: #172

k8s-ci-robot · 2020-09-17T18:53:35Z

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.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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.

k8s-ci-robot · 2020-09-17T18:53:39Z

Welcome @christarazi!

It looks like this is your first PR to kubernetes-sigs/structured-merge-diff 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/structured-merge-diff has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-09-17T18:53:39Z

Hi @christarazi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

The intention of using this library is to utilize the facility to convert Kubernetes objects to `unstructured.Unstructured`. Previously, we were doing this by hand. For an explanation for why we are using a fork, read below. This commit migrates our use of sigs.k8s.io/structured-merge-diff/v4 over to the fork at github.com/christarazi/structured-merge-diff/v4 because of kubernetes-sigs/structured-merge-diff#172. The fork contains the fix (kubernetes-sigs/structured-merge-diff#173) which we are vendoring in. Once the issue is fixed or the upstream PR is merged, we can revert back to using the original library. Signed-off-by: Chris Tarazi <[email protected]>

apelisse · 2020-09-18T16:44:08Z

/assign @jpbetz

apelisse · 2020-09-18T16:44:19Z

/ok-to-test

jpbetz · 2020-09-18T17:11:06Z

The fix looks right to me. Would you add a test to ensure this doesn't get broken in the future? It should be as simple as adding a new tests struct and case here:

structured-merge-diff/value/valuereflect_test.go

Line 241 in 1cc6019

func TestReflectStruct(t *testing.T) {

The intention of using this library is to utilize the facility to convert Kubernetes objects to `unstructured.Unstructured`. Previously, we were doing this by hand. For an explanation for why we are using a fork, read below. This commit migrates our use of sigs.k8s.io/structured-merge-diff/v4 over to the fork at github.com/christarazi/structured-merge-diff/v4 because of kubernetes-sigs/structured-merge-diff#172. The fork contains the fix (kubernetes-sigs/structured-merge-diff#173) which we are vendoring in. Once the issue is fixed or the upstream PR is merged, we can revert back to using the original library. Signed-off-by: Chris Tarazi <[email protected]>

timebertt · 2020-10-15T13:08:43Z

@christarazi what's the status on this?
We have also hit this issue (ref gardener/gardener#2977) and would be very interested in having a upstream fix.

christarazi · 2020-10-22T00:06:11Z

@timebertt Apologies, yes this is still on my radar. Just trying to find the time :). Hopefully in the next week or so.

Signed-off-by: Chris Tarazi <[email protected]>

This commit converts an embedded struct into its original type if it is a pointer. Fixes: kubernetes-sigs#172 Signed-off-by: Chris Tarazi <[email protected]>

christarazi · 2020-10-29T02:56:00Z

I signed it

apelisse · 2020-10-29T15:18:57Z

/unassign

apelisse · 2020-10-29T15:19:08Z

ping @jpbetz

ialidzhikov · 2020-11-19T15:17:41Z

/kind bug
/priority important-soon

@jpbetz, could you please recheck?

apelisse · 2020-11-24T16:09:29Z

/lgtm
/approve

k8s-ci-robot · 2020-11-24T16:09:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, christarazi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [apelisse]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Cherry pick of #173: Fix panic on pointer to embedded struct

…-struct Cherry pick of #173: Fix panic on pointer to embedded struct

This updates the dependency to 9f9c77085dec, as it was previously a temporary fork. This includes the fix to kubernetes-sigs/structured-merge-diff#173. At the time of this commit, there hasn't been a release made including this fix, so we are explicitly using the latest commit. Signed-off-by: Chris Tarazi <[email protected]>

Following a new release which includes kubernetes-sigs/structured-merge-diff#173, we no longer need to pin to a commit on master. Update to use an officially released version. Signed-off-by: Chris Tarazi <[email protected]>

[ upstream commit 28a8b47 ] This updates the dependency to 9f9c77085dec, as it was previously a temporary fork. This includes the fix to kubernetes-sigs/structured-merge-diff#173. At the time of this commit, there hasn't been a release made including this fix, so we are explicitly using the latest commit. Signed-off-by: Chris Tarazi <[email protected]> Signed-off-by: André Martins <[email protected]>

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 17, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 17, 2020

k8s-ci-robot requested review from apelisse and lavalamp September 17, 2020 18:53

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 17, 2020

k8s-ci-robot assigned jpbetz Sep 18, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 18, 2020

Fix incorrect godoc on GetFrom

e5a286e

Signed-off-by: Chris Tarazi <[email protected]>

christarazi force-pushed the fix-panic branch from 1cc6019 to 763ad4e Compare October 29, 2020 02:42

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 29, 2020

Fix panic on pointer to embedded struct

076ef8d

This commit converts an embedded struct into its original type if it is a pointer. Fixes: kubernetes-sigs#172 Signed-off-by: Chris Tarazi <[email protected]>

christarazi force-pushed the fix-panic branch from 763ad4e to 076ef8d Compare October 29, 2020 02:53

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 29, 2020

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 19, 2020

k8s-ci-robot assigned apelisse Nov 24, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2020

k8s-ci-robot merged commit 9f9c770 into kubernetes-sigs:master Nov 24, 2020

ialidzhikov mentioned this pull request Nov 24, 2020

Cherry pick of #173: Fix panic on pointer to embedded struct #180

Merged

k8s-ci-robot added a commit that referenced this pull request Nov 24, 2020

Merge pull request #180 from ialidzhikov/fix/panic-on-embedded-struct

f5fd4ea

Cherry pick of #173: Fix panic on pointer to embedded struct

christarazi deleted the fix-panic branch November 24, 2020 18:58

This was referenced Jan 14, 2021

Vendor k/*@v0.19 and [email protected] gardener/gardener#3393

Merged

Cherry pick of #173: Fix panic on pointer to embedded struct #182

Merged

k8s-ci-robot added a commit that referenced this pull request Jan 14, 2021

Merge pull request #182 from ialidzhikov/cherrypick/panic-on-embedded…

c021244

…-struct Cherry pick of #173: Fix panic on pointer to embedded struct

christarazi mentioned this pull request Jan 27, 2021

vendor: Update sigs.k8s.io/structured-merge-diff/v4 cilium/cilium#14752

Merged

christarazi mentioned this pull request Mar 27, 2021

vendor: Update sigs.k8s.io/structured-merge-diff to v4.1.0 cilium/cilium#15488

Merged

apelisse mentioned this pull request Apr 16, 2021

Pin sigs.k8s.io/structured-merged-diff/v3 at v3.0.1 kubernetes/kubernetes#101179

Merged

Fix panic on pointer to embedded struct #173

Fix panic on pointer to embedded struct #173

Uh oh!

Conversation

christarazi commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Sep 17, 2020

Uh oh!

k8s-ci-robot commented Sep 17, 2020

Uh oh!

k8s-ci-robot commented Sep 17, 2020

Uh oh!

apelisse commented Sep 18, 2020

Uh oh!

apelisse commented Sep 18, 2020

Uh oh!

jpbetz commented Sep 18, 2020

Uh oh!

timebertt commented Oct 15, 2020

Uh oh!

christarazi commented Oct 22, 2020

Uh oh!

christarazi commented Oct 29, 2020

Uh oh!

apelisse commented Oct 29, 2020

Uh oh!

apelisse commented Oct 29, 2020

Uh oh!

ialidzhikov commented Nov 19, 2020

Uh oh!

apelisse commented Nov 24, 2020

Uh oh!

k8s-ci-robot commented Nov 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

christarazi commented Sep 17, 2020 •

edited

Loading