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 fieldmanager tests for stripFields #74207

Merged
merged 3 commits into from
Feb 21, 2019
Merged

add fieldmanager tests for stripFields #74207

merged 3 commits into from
Feb 21, 2019

Conversation

kwiesmueller
Copy link
Member

What type of PR is this?
/kind bug

What this PR does / why we need it:

Adds tests for FieldManager to check the stripFields functionality is working as expected.

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 18, 2019
@kwiesmueller
Copy link
Member Author

/cc @apelisse @jennybuckley

@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 18, 2019
apelisse
apelisse reviewed Feb 18, 2019
View reviewed changes
Copy link
Member

@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.

I'm curious what else we should test here? What's the important logic of fieldmanager in particular that we want to verify?

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go Outdated
"metadata": {
"name": "b",
"namespace": "b",
"creationTimestamp": "`+time.Now().UTC().Format(time.RFC3339)+`",
Copy link
Member

Choose a reason for hiding this comment

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

Then you don't have to build the test dynamically, which is an anti-pattern

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go Show resolved Hide resolved
@apelisse apelisse mentioned this pull request Feb 19, 2019
Merged
@kwiesmueller
Copy link
Member Author

/retest

apelisse
apelisse reviewed Feb 20, 2019
View reviewed changes
Copy link
Member

@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.

I'm not entirely happy with this yet, but I think we can merge it and improve later (it's already an improvement). I think it's very focused on "stripping the fields", which is clearly not the sole purpose of FieldManager as far as I can remember, but that's useful.
We still may want a smaller test just for testing that specific stripping functionality.

/lgtm
/approve

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go
"apiVersion": "v1",
"kind": "Deployment",
"metadata": {
"name": "b",
Copy link
Member

Choose a reason for hiding this comment

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

That really shouldn't happen though (applying with a different name/namespace), it might not matter in this test but that's confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, it's my only way of testing if the fields get stripped on changes. Or is there another one?

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go
func TestApplyStripsFields(t *testing.T) {
f := NewTestFieldManager(t)

obj := &corev1.Pod{
Copy link
Member

Choose a reason for hiding this comment

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

what happens if you keep this empty?

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Feb 20, 2019
@kwiesmueller
Copy link
Member Author

Sure, we would want even more. My goal was to get tests for strip fields first and at least create the first test setup for it.
We definitely should go further from there.

@kwiesmueller kwiesmueller changed the title [WIP] add fieldmanager tests for stripFields add fieldmanager tests for stripFields Feb 20, 2019
@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 Feb 20, 2019
@k8s-ci-robot k8s-ci-robot merged commit 8ca0d8c into kubernetes:master Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apelisse apelisse apelisse left review comments

@jennybuckley jennybuckley Awaiting requested review from jennybuckley

@CaoShuFeng CaoShuFeng Awaiting requested review from CaoShuFeng

@dchen1107 dchen1107 Awaiting requested review from dchen1107

Assignees

@apelisse apelisse

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kwiesmueller @k8s-ci-robot @apelisse