✨ testdata/cronjob_types.go: add NestedStructWithSeveralFieldsDoubleMarked #704

jan--f · 2022-07-06T13:07:32Z

This adds a field to the cronjob test data, that ensures that fields
which are marked twice (once through a type marker, one through a field
marker) still generate the correct yaml structure.

Signed-off-by: Jan Fajerski jfajersk@redhat.com

…everalFields The type `NestedStructWithSeveralFields` is already annotated. Annotating the field of the same name is not necessary, might confuse user looking at this example and potentially cover up future regressions (the field annotation might cover up the type annotation not having the wanted effect). Signed-off-by: Jan Fajerski <jfajersk@redhat.com>

k8s-ci-robot · 2022-07-06T13:07:40Z

Welcome @jan--f!

It looks like this is your first PR to kubernetes-sigs/controller-tools 🎉. 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/controller-tools 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 · 2022-07-06T13:07:40Z

Hi @jan--f. 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.

jan--f · 2022-07-06T13:12:06Z

This is a tiny (but I think worthwhile) fix, following up on #692. Hence ping @chrischdi and @FillZpp

chrischdi

/lgtm

FillZpp · 2022-07-07T03:40:56Z

I'm not sure, should we keep this test to verify that annotated on both field and type, to avoid bugs like these #688 and #693 (comment)

jan--f · 2022-07-07T11:19:01Z

I'm not sure, should we keep this test to verify that annotated on both field and type, to avoid bugs like these #688 and #693 (comment)

Yes that makes sense, I added a commit to that effect. WDYT?

jan--f · 2022-08-23T12:42:39Z

ping @FillZpp Any further concerns about this?

FillZpp · 2022-08-24T02:54:38Z

@jan--f I don't understand what's the difference between NestedStructWithSeveralFields and NestedStructWithSeveralFieldsDoubleMarked ?

This adds a field to the cronjob test data, that ensures that fields which are marked twice (once through a type marker, one through a field marker) still generate the correct yaml structure. Signed-off-by: Jan Fajerski <jfajersk@redhat.com>

jan--f · 2022-08-24T06:35:24Z

@jan--f I don't understand what's the difference between NestedStructWithSeveralFields and NestedStructWithSeveralFieldsDoubleMarked ?

@FillZpp Right, because I forgot to add the field marker. Added it now, thank you!

edit: I also updated the title and PR description.

FillZpp · 2022-08-24T09:16:11Z

/ok-to-test

FillZpp · 2022-08-25T03:31:59Z

/lgtm

jan--f · 2022-09-23T12:17:51Z

@chrischdi anything still blocking this?

chrischdi · 2022-09-23T13:44:01Z

/lgtm

from my side. But requires a maintainer for approval (which I am not).

jan--f · 2022-09-23T14:08:27Z

Thanks, let me try @pwittrock and @mengqiy for an approve then.

camilamacedo86 · 2022-11-17T09:23:55Z

pkg/crd/testdata/cronjob_types.go

+	// A struct that can only be entirely replaced via a nested type and
+	// field markers.
+	// +structType=atomic
+	NestedStructWithSeveralFieldsDoubleMarked NestedStructWithSeveralFields `json:"nestedStructWithSeveralFieldsDoubleMarked"`


It seems a workarround. Could you please raise an issue or add in the description:

a) what you were doing when you face the error ( with an example code )
b) what do you expected to see ?
c) what was the behaviour faced instead?

Also, if we move forward here I think we need a better name than FieldsDoubleMarked. DoubleMarket with what? (i.e. NestedStructWithSeveralFieldsMarkedWithAB)

The context of this is #676, which is a duplicate of #435.

@camilamacedo86 so this just adds an additional test case. NestedStructWithSeveralFields ensures that the resulting manifest carries the atomic marker from the type. NestedStructWithSeveralFieldsDoubleMarked ensures the same thing only that the marker is applied to the type, as well as the to the field of the struct.

camilamacedo86 · 2022-11-17T09:24:48Z

pkg/crd/testdata/cronjob_types.go

@@ -162,9 +162,13 @@ type CronJobSpec struct {
 	StructWithSeveralFields NestedObject `json:"structWithSeveralFields"`

 	// A struct that can only be entirely replaced via a nested type.
-	// +structType=atomic


Why it was removed?

It was added the the new declaration below.

camilamacedo86

It seems requires more info first.

alvaroaleman · 2023-04-22T14:59:48Z

/label tide/merge-method-squash

k8s-ci-robot · 2023-04-22T15:00:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, jan--f

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~~ [alvaroaleman]

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

alvaroaleman · 2023-04-22T18:16:24Z

Thanks!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 6, 2022

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 Jul 6, 2022

k8s-ci-robot requested review from mengqiy and pwittrock July 6, 2022 13:07

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 6, 2022

jan--f mentioned this pull request Jul 6, 2022

[struct,list,map]Type markers on types are ignored #676

Closed

chrischdi reviewed Jul 6, 2022

View reviewed changes

k8s-ci-robot assigned chrischdi Jul 6, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2022

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 7, 2022

jan--f requested a review from chrischdi July 14, 2022 11:29

jan--f force-pushed the drop-atomic-nested-type-annotation branch from 04d17a3 to b25a1f6 Compare August 24, 2022 06:33

jan--f changed the title ~~✨ testdata/cronjob_types.go: drop field annotation of NestedStructWithSeveralFields~~ ✨ testdata/cronjob_types.go: add NestedStructWithSeveralFieldsDoubleMarked Aug 24, 2022

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 Aug 24, 2022

k8s-ci-robot assigned FillZpp Aug 25, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2022

camilamacedo86 reviewed Nov 17, 2022

View reviewed changes

camilamacedo86 suggested changes Nov 17, 2022

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 22, 2023

alvaroaleman approved these changes Apr 22, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2023

k8s-ci-robot merged commit 9b9ec3b into kubernetes-sigs:master Apr 22, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ testdata/cronjob_types.go: add NestedStructWithSeveralFieldsDoubleMarked #704

✨ testdata/cronjob_types.go: add NestedStructWithSeveralFieldsDoubleMarked #704

jan--f commented Jul 6, 2022 •

edited

k8s-ci-robot commented Jul 6, 2022

k8s-ci-robot commented Jul 6, 2022

jan--f commented Jul 6, 2022 •

edited

chrischdi left a comment

FillZpp commented Jul 7, 2022

jan--f commented Jul 7, 2022

jan--f commented Aug 23, 2022

FillZpp commented Aug 24, 2022

jan--f commented Aug 24, 2022 •

edited

FillZpp commented Aug 24, 2022

FillZpp commented Aug 25, 2022

jan--f commented Sep 23, 2022

chrischdi commented Sep 23, 2022

jan--f commented Sep 23, 2022

camilamacedo86 Nov 17, 2022

jan--f Jan 24, 2023

jan--f Jan 24, 2023

camilamacedo86 Nov 17, 2022

jan--f Jan 24, 2023

camilamacedo86 left a comment

alvaroaleman commented Apr 22, 2023

k8s-ci-robot commented Apr 22, 2023

alvaroaleman commented Apr 22, 2023

✨ testdata/cronjob_types.go: add NestedStructWithSeveralFieldsDoubleMarked #704

✨ testdata/cronjob_types.go: add NestedStructWithSeveralFieldsDoubleMarked #704

Conversation

jan--f commented Jul 6, 2022 • edited

k8s-ci-robot commented Jul 6, 2022

k8s-ci-robot commented Jul 6, 2022

jan--f commented Jul 6, 2022 • edited

chrischdi left a comment

Choose a reason for hiding this comment

FillZpp commented Jul 7, 2022

jan--f commented Jul 7, 2022

jan--f commented Aug 23, 2022

FillZpp commented Aug 24, 2022

jan--f commented Aug 24, 2022 • edited

FillZpp commented Aug 24, 2022

FillZpp commented Aug 25, 2022

jan--f commented Sep 23, 2022

chrischdi commented Sep 23, 2022

jan--f commented Sep 23, 2022

camilamacedo86 Nov 17, 2022

Choose a reason for hiding this comment

jan--f Jan 24, 2023

Choose a reason for hiding this comment

jan--f Jan 24, 2023

Choose a reason for hiding this comment

camilamacedo86 Nov 17, 2022

Choose a reason for hiding this comment

jan--f Jan 24, 2023

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

alvaroaleman commented Apr 22, 2023

k8s-ci-robot commented Apr 22, 2023

alvaroaleman commented Apr 22, 2023

jan--f commented Jul 6, 2022 •

edited

jan--f commented Jul 6, 2022 •

edited

jan--f commented Aug 24, 2022 •

edited