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.

Sign up for GitHub

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

Jump to bottom

add field-level element relationship which overrides referred type #220

Merged
merged 5 commits into from
Jul 30, 2022
Merged

add field-level element relationship which overrides referred type #220

merged 5 commits into from
Jul 30, 2022

Conversation

alexzielenski
Copy link
Contributor

Part of our solution for kubernetes/kubernetes#110495

Adds necessary functionality to support field-level annotations which may override the annotation of the referred type.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 22, 2022
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 22, 2022
@alexzielenski alexzielenski changed the title add field-level element relationship which overrides referred type [WIP] add field-level element relationship which overrides referred type Jun 22, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2022
@alexzielenski alexzielenski mentioned this pull request Jun 22, 2022
Merged
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 22, 2022
@alexzielenski
Copy link
Contributor Author

/cc @Jefftree

@alexzielenski alexzielenski changed the title [WIP] add field-level element relationship which overrides referred type add field-level element relationship which overrides referred type Jun 24, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2022
apelisse
apelisse reviewed Jun 24, 2022
View reviewed changes
Copy link
Contributor

@apelisse apelisse left a comment

Choose a reason for hiding this comment

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

We need tests for that, thanks

schema/elements.go Outdated Show resolved Hide resolved
schema/elements.go Outdated Show resolved Hide resolved
@alexzielenski
Copy link
Contributor Author

Added tests, addressed comments.

apelisse
apelisse reviewed Jul 2, 2022
View reviewed changes
Copy link
Contributor

@apelisse apelisse left a comment •
edited

Choose a reason for hiding this comment

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

I think I'd like to see a more sophisticated test to show that this works correctly, take an example of a test in merge/ where you can specify the schema and the operations, make sure they work properly.

schema/elements.go Outdated
}

default:
// Atom violates invariant that one of Map/List/Scalar is set
Copy link
Contributor

Choose a reason for hiding this comment

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

This empty Atom return value is just terrible. I don't think we handle it properly anywhere and usually leads to hard to debug errors. Anyways, just ranting :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the contract of this function is to not inspect the first return value of the second is false.

schema/elements.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 6, 2022
@alexzielenski
Copy link
Contributor Author

tests added, newline removed ;)

lavalamp
lavalamp reviewed Jul 14, 2022
View reviewed changes
Copy link
Contributor

@lavalamp lavalamp left a comment

Choose a reason for hiding this comment

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

I guess you probably considered duplicating+renaming types as needed instead of this?

schema/elements.go Show resolved Hide resolved
schema/elements.go Outdated Show resolved Hide resolved
lavalamp
lavalamp reviewed Jul 26, 2022
View reviewed changes
schema/elements.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 26, 2022
@alexzielenski
Copy link
Contributor Author

alexzielenski commented Jul 26, 2022 •
edited

/restest

I double checked the diff from that gofmt error. My checkout does not contain those changes. I tried to re-run gofmt locally but saw no changes. Not seeing the same thing on my side... Maybe the CI is confused?

It is expecting changes in comments replacing * with -

@alexzielenski
Copy link
Contributor Author

/retest

Had typo in last attempt.

lavalamp
lavalamp reviewed Jul 28, 2022
View reviewed changes
typed/typed.go Outdated
// value.
// - Container typed elements will have their items ordered:
// - like tv, if pso doesn't change anything in the container
// - like pso, if pso does change something in the container.
Copy link
Contributor

Choose a reason for hiding this comment

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

The extra indenting in the original was deliberate...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately gofmt won't allow this anymore so I was forced to change it to an ordered list.

lavalamp
lavalamp reviewed Jul 28, 2022
View reviewed changes
typed/reconcile_schema.go Outdated Show resolved Hide resolved
@alexzielenski
fix gofmt
c779eba 
unfortunately cant nest indentation with an unordered list
@lavalamp
Copy link
Contributor

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, lavalamp

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2022
@k8s-ci-robot k8s-ci-robot merged commit ebd569f into kubernetes-sigs:master Jul 30, 2022
@apelisse
Copy link
Contributor

I tagged the merged commit with v4.2.2.

@apelisse
Copy link
Contributor

I suspect the schema change to pointer is backwards incompatible, I'll have to fix the tag and change the release branch to v5.

@apelisse
Copy link
Contributor

apelisse commented Aug 1, 2022

I removed the v4.2.2 tag since I think it was not correct semver.

@seankhliao seankhliao mentioned this pull request Aug 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@apelisse apelisse apelisse left review comments

@jennybuckley jennybuckley Awaiting requested review from jennybuckley

@Jefftree Jefftree Awaiting requested review from Jefftree

Assignees

@lavalamp lavalamp

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alexzielenski @lavalamp @k8s-ci-robot @apelisse