Skip to content

Populate OpenAPI in all integration tests#107765

Merged
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
Jefftree:ssa-integration-enable
Jan 26, 2022
Merged

Populate OpenAPI in all integration tests#107765
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
Jefftree:ssa-integration-enable

Conversation

@Jefftree

@Jefftree Jefftree commented Jan 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

SSA is enabled on an as needed basis in integration tests by populating the OpenAPI field of the configuration. APF recently switched to use SSA in their controller, and SSA is now needed in all integration tests. This PR populates the OpenAPI in all integration tests.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@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/S Denotes a PR that changes 10-29 lines, ignoring generated files. 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. labels Jan 25, 2022
@k8s-ci-robot

k8s-ci-robot commented Jan 25, 2022

Copy link
Copy Markdown
Contributor

@Jefftree: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 25, 2022
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 25, 2022
@Jefftree Jefftree changed the title [WIP] Populate OpenAPI in all integration tests Populate OpenAPI in all integration tests Jan 25, 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 Jan 25, 2022
@Jefftree Jefftree force-pushed the ssa-integration-enable branch from 8e6bf93 to 7867bb5 Compare January 25, 2022 20:27
@Jefftree Jefftree mentioned this pull request Jan 25, 2022
Closed
@wojtek-t

wojtek-t commented Jan 25, 2022

Copy link
Copy Markdown
Member

/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 Jan 25, 2022
@k8s-ci-robot

k8s-ci-robot commented Jan 25, 2022

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jefftree, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Jan 25, 2022
@liggitt

liggitt commented Jan 25, 2022

Copy link
Copy Markdown
Member

if we always want/expect FieldManager to be set, should this part of the server setup become unconditional and stop tolerating nil OpenAPIModels?

	if a.group.OpenAPIModels != nil && utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply) {

liggitt
liggitt reviewed Jan 25, 2022
View reviewed changes
Comment thread test/integration/framework/controlplane_utils.go Outdated
@Jefftree Jefftree force-pushed the ssa-integration-enable branch from 7867bb5 to eb8f6fe Compare January 25, 2022 22:16
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2022
@liggitt

liggitt commented Jan 25, 2022

Copy link
Copy Markdown
Member

/lgtm

did you check the integration test log to make sure no other server setup paths needed adjusting?

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

Jefftree commented Jan 25, 2022

Copy link
Copy Markdown
Member Author

if we always want/expect FieldManager to be set, should this part of the server setup become unconditional and stop tolerating nil OpenAPIModels?

	if a.group.OpenAPIModels != nil && utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply) {

I see @apelisse has already given the thumbs up. My instinct is also yes but I'd like to do a bit more investigation around that.

@liggitt

liggitt commented Jan 25, 2022

Copy link
Copy Markdown
Member

I see @apelisse has already given the thumbs up. My instinct is also yes but I'd like to do a bit more investigation around that.

fine to do in a follow up

@k8s-ci-robot k8s-ci-robot merged commit 6a1de6b into kubernetes:master Jan 26, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Jan 26, 2022
@apelisse

apelisse commented Jan 26, 2022

Copy link
Copy Markdown
Member

Another thing we have is that we enable the server-side apply feature in a lot of the ssa tests, should we remove these calls?

@Jefftree

Jefftree commented Jan 26, 2022

Copy link
Copy Markdown
Member Author

SSA was GA in 1.22. Is there a minimum number of releases we need to wait before removing the feature flag?

@apelisse

apelisse commented Jan 26, 2022

Copy link
Copy Markdown
Member

Well, I'm not even mentioning removing the feature flag (I've seen discussion that might answer this question, but I'm not sure, I think it possibly breaks the command-line), but the tests can probably now assume that the feature is enabled?

@liggitt liggitt mentioned this pull request Jan 26, 2022
Merged
@liggitt

liggitt commented Jan 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

the integration test job started timing out dramatically last night (>50% of runs), trying to figure out if this is related:
https://testgrid.k8s.io/presubmits-kubernetes-blocking#pull-kubernetes-integration

@Jefftree Jefftree mentioned this pull request Jan 26, 2022
Merged
@liggitt liggitt mentioned this pull request Jan 29, 2022
Closed
@MikeSpreitzer

MikeSpreitzer commented Jan 31, 2022

Copy link
Copy Markdown
Member

@MikeSpreitzer

@Jefftree Jefftree deleted the ssa-integration-enable branch December 2, 2022 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

@johnSchnake johnSchnake Awaiting requested review from johnSchnake

@neolit123 neolit123 Awaiting requested review from neolit123

Assignees

@liggitt liggitt

@wojtek-t wojtek-t

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test 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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

v1.24

Development

Successfully merging this pull request may close these issues.

6 participants

@Jefftree @k8s-ci-robot @wojtek-t @liggitt @apelisse @MikeSpreitzer