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

managedfields: A few improvements which will make testing easier #116973

Merged
merged 5 commits into from
May 16, 2023
Merged

managedfields: A few improvements which will make testing easier #116973

merged 5 commits into from
May 16, 2023

Conversation

apelisse
Copy link
Member

Allows creating a typeconverter from a client (i.e. by taking the data of the client and formatting it so that one can create a type converter).

I'm trying to see if that's what we want, possible alternatives:

  • We create that from the Root object
  • We just make a function to merge the specs
  • ...?

This needs tests.

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Nothing

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

/assign @alexzielenski

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 28, 2023
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Mar 28, 2023
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.27 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.27.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Mar 28 16:29:22 UTC 2023.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 28, 2023
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 28, 2023
@apelisse
Copy link
Member Author

/hold

This is still a WIP, not sure if we want that.

@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 Mar 29, 2023
@cici37
Copy link
Contributor

cici37 commented Mar 30, 2023

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 30, 2023
@apelisse
Copy link
Member Author

apelisse commented May 1, 2023

In tests specifically, we usually need to create a fieldmanager from a test client (either the embedded or the one that loads from files). This creates the testConverter and then you can use it to create a specific field manager for test. It could be even more convenient but I'm not quite sure what the right thing to do is here.

@apelisse apelisse force-pushed the add-client-to-typeconverter branch from 7eeba28 to e342d11 Compare May 16, 2023 18:28
@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 May 16, 2023
@apelisse apelisse changed the title openapi: Create client -> TypeConverter function managedfields: A few improvements which will make testing easier May 16, 2023
alexzielenski
alexzielenski approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@alexzielenski alexzielenski left a comment

Choose a reason for hiding this comment

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

/lgtm

version check refactors seems orthogonal but looks OK. NewFakeFieldManager and NewTypeConverter lgtm

staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/versioncheck.go

// NewVersionCheckManager creates a manager that makes sure that the
// applied object is in the proper version.
func NewVersionCheckManager(fieldManager Manager, gvk schema.GroupVersionKind) Manager {
Copy link
Contributor

Choose a reason for hiding this comment

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

how does version check manager help testability. seems like an orthogonal change

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, so, in tests, it's very unlikely that we will have one FieldManager per individual type like we do in the apiserver. And at least the SkipProbababilityManager (for now, but I expect more are going to have the same problem) was struggling to create a new object because of that. Hiding the GVK makes some things easier.

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

LGTM label has been added.

Git tree hash: be8c24e68da0463d51591e347f184ffb69d8bad0

@apelisse
openapi: Create client -> TypeConverter function
45853e2 
Allows creating a typeconverter from a client (i.e. by taking the data
of the client and formatting it so that one can create a type
converter).
@apelisse
managedfields: Create NewFakeFieldManager
559d12f 
And simplify how a lot of the fakes are created. Notably, the converter
was never really used to do anything so this is simplified.
@apelisse
managedfields: Improve/strengthen version checking for Apply
b52e57c
@apelisse
managedfields: Remove gvk from SkipNonApplied
15c5713 
Let's remove the dependency on the GVK in SkipNonApplied internal
manager, since we can deduce the type from the given object.
@apelisse
Update vendor
b81cfb9
@apelisse apelisse force-pushed the add-client-to-typeconverter branch from e342d11 to b81cfb9 Compare May 16, 2023 20:11
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2023
@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 16, 2023
seans3
seans3 approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@seans3 seans3 left a comment

Choose a reason for hiding this comment

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

Nit on documentation. You can fix it next time if you agree.

/lgtm
/approve

staging/src/k8s.io/client-go/openapi/typeconverter.go
"k8s.io/kube-openapi/pkg/validation/spec"
)

func NewTypeConverter(client Client, preserveUnknownFields bool) (managedfields.TypeConverter, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should there be documentation for this function?

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

LGTM label has been added.

Git tree hash: f2438b669f22972f7f2f2e50a47c8cac2bfb63a6

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, apelisse, arimallick, seans3

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 May 16, 2023
@apelisse
Copy link
Member Author

/hold cancel

I think we're good with this.

@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 May 16, 2023
@k8s-ci-robot k8s-ci-robot merged commit addf9d4 into kubernetes:master May 16, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seans3 seans3 seans3 approved these changes

@arimallick arimallick arimallick approved these changes

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@sttts sttts Awaiting requested review from sttts

@alexzielenski alexzielenski Awaiting requested review from alexzielenski

Assignees

@alexzielenski alexzielenski

@seans3 seans3

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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.
6 participants
@apelisse @k8s-ci-robot @cici37 @alexzielenski @seans3 @arimallick