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

🌱 Refactor patch helper to handle observedGeneration and Conditions #3118

Merged
merged 1 commit into from
Jun 4, 2020
Merged

🌱 Refactor patch helper to handle observedGeneration and Conditions #3118

merged 1 commit into from
Jun 4, 2020

Conversation

vincepri
Copy link
Member

@vincepri vincepri commented May 29, 2020 •
edited
Loading

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:
This PR is a major refactor of our test utilities to

  • Automatically update observedGeneration via an Option that users might specify when calling NewHelper.
  • Automatically detect if the object passed in conforms to conditions.{Getter, Setter} interfaces and try to remedy possible conflicts when we have multiple controllers updating conditions at the same time.
  • Increase our tests coverage and use envtest (a real local api server) instead of a fake client.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 29, 2020
@vincepri vincepri force-pushed the wip-patch-observed branch 3 times, most recently from 523bd53 to 3a6ba7b Compare May 29, 2020 19:56
@vincepri vincepri changed the title [WIP] Add options to patch helper for ObservedGeneration 🏃 Add options to patch helper for ObservedGeneration May 29, 2020
@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 May 29, 2020
detiber
detiber reviewed May 29, 2020
View reviewed changes
util/patch/patch.go Show resolved Hide resolved
util/patch/patch.go Outdated Show resolved Hide resolved
@vincepri vincepri changed the title 🏃 Add options to patch helper for ObservedGeneration [WIP] 🏃 Add options to patch helper for ObservedGeneration & Conditions May 29, 2020
@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 May 29, 2020
@vincepri vincepri force-pushed the wip-patch-observed branch from 3a6ba7b to 41e48bf Compare May 29, 2020 21:43
@vincepri
Copy link
Member Author

/hold

We shouldn't merge this until there is more testing and controller-runtime has a release (next week)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2020
@vincepri vincepri force-pushed the wip-patch-observed branch from 41e48bf to c28e000 Compare May 29, 2020 21:47
@vincepri
Copy link
Member Author

/assign @detiber @ncdc

@vincepri vincepri force-pushed the wip-patch-observed branch from c28e000 to 0501ce6 Compare May 29, 2020 23:07
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 29, 2020
@vincepri
Copy link
Member Author

/assign @fabriziopandini @benmoss

@vincepri
Copy link
Member Author

vincepri commented May 29, 2020 •
edited
Loading

This has been reworked quite a bit now, and the flow should be more clear and robust.

@vincepri vincepri force-pushed the wip-patch-observed branch from 0501ce6 to 10b3d1c Compare May 29, 2020 23:36
@vincepri
Copy link
Member Author

vincepri commented Jun 1, 2020

/test pull-cluster-api-make

@vincepri vincepri force-pushed the wip-patch-observed branch from 963c5fd to 8f3e45e Compare June 1, 2020 22:55
@CecileRobertMichon
Copy link
Contributor

@vincepri could you please add a PR description describing what this is doing and why?

@vincepri vincepri force-pushed the wip-patch-observed branch from 8f3e45e to c535cfd Compare June 1, 2020 23:22
@vincepri
Copy link
Member Author

vincepri commented Jun 1, 2020

Description updated, I think this is ready for review, waiting for the e2e to pass first.

@vincepri vincepri force-pushed the wip-patch-observed branch 2 times, most recently from a8b3d05 to 8a9155f Compare June 2, 2020 01:42
JoelSpeed
JoelSpeed reviewed Jun 2, 2020
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

LGTM, deferring to other reviewers as there's been a lot of eyes on this PR

@vincepri vincepri force-pushed the wip-patch-observed branch from 8a9155f to 2071d84 Compare June 2, 2020 16:20
@vincepri
Copy link
Member Author

vincepri commented Jun 2, 2020

Added more tests around conditions and conflict resolution

@vincepri
Copy link
Member Author

vincepri commented Jun 2, 2020

/milestone v0.3.7

@k8s-ci-robot k8s-ci-robot added this to the v0.3.7 milestone Jun 2, 2020
@vincepri
Copy link
Member Author

vincepri commented Jun 2, 2020

/retitle 🌱 Refactor patch helper to handle observedGeneration and Conditions

@k8s-ci-robot k8s-ci-robot changed the title 🏃 Refactor patch helper to handle observedGeneration and Conditions 🌱 Refactor patch helper to handle observedGeneration and Conditions Jun 2, 2020
@CecileRobertMichon
Copy link
Contributor

lgtm

@benmoss benmoss mentioned this pull request Jun 3, 2020
Merged
@vincepri vincepri force-pushed the wip-patch-observed branch from 2071d84 to a7cf565 Compare June 4, 2020 18:36
@benmoss benmoss mentioned this pull request Jun 4, 2020
Closed
@vincepri vincepri force-pushed the wip-patch-observed branch from a7cf565 to 5e1bf5d Compare June 4, 2020 20:01
@vincepri
Copy link
Member Author

vincepri commented Jun 4, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2020
@vincepri
Refactor patch helper to handle observedGeneration and Conditions
d09777b 
- Includes a new test suite to provide better coverage
  scenarios, and avoid the fake client.
- Includes the use of a strategic 3 way merge when dealing
  with cluster-api conditions.

Signed-off-by: Vince Prignano <[email protected]>
@vincepri vincepri force-pushed the wip-patch-observed branch from 5e1bf5d to d09777b Compare June 4, 2020 20:06
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 4, 2020 •
edited
Loading

@vincepri: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-apidiff d09777b link /test pull-cluster-api-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@vincepri
Copy link
Member Author

vincepri commented Jun 4, 2020

/test pull-cluster-api-test

@CecileRobertMichon
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit d54acb5 into kubernetes-sigs:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed JoelSpeed left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@detiber detiber detiber left review comments

@benmoss benmoss benmoss left review comments

Assignees

@ncdc ncdc

@benmoss benmoss

@detiber detiber

@fabriziopandini fabriziopandini

@CecileRobertMichon CecileRobertMichon

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
v0.3.7
Development

Successfully merging this pull request may close these issues.
8 participants
@vincepri @k8s-ci-robot @CecileRobertMichon @benmoss @detiber @fabriziopandini @JoelSpeed @ncdc