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.

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

cherry pick #100341 #98576 #95836 on 1.18 to enable SSA with APIService #100715

Conversation

kevindelgado
Copy link
Contributor

@kevindelgado kevindelgado commented Mar 31, 2021

Cherry pick of #100341 #98576 #95836 on release-1.18.

#100341: Add ability to skip OpenAPI handler installation

  • The line that skips open API Installation in the kube-aggregator has been removed because OpenAPI generation does not exist in kube-aggregator until 1.19.

#98576: Declare TCP default for service port protocol

  • The commit declaring TCP as the default protocol has been removed because it uses features implemented in Add defaults openapi #96480 that do not exist in 1.18.
  • A line has been added to set the protocol of the service object in the etcd data stub in order to account for SSA's inability to set a default protocol in 1.18

#95836: Add yaml util to unmarshal numbers into int/float

fixes #99046

Fixes using server-side apply with APIService resources

@kevindelgado
Copy link
Contributor Author

kevindelgado commented Mar 31, 2021

Note that the ability to skip OpenAPI handler installation in the standalone kube-aggregator was removed from this cherry-pick compared to the 1.19 and 1.20 cherry-pick(just this one line) because OpenAPI generation was erroneously missing from kube-aggregator until 1.19 (and fixed via these two PRs (#91537 and #91525).

I figure that it makes the most sense to just drop this single conflicting line from the cherry-pick instead of

a) Also cherry-picking the two (fairly large) standalone kube-aggregator OpenAPI generation PRs or
b) Abandoning the cherry-pick for 1.18 entirely.

Let me know if you’d prefer one of those options (or another one entirely)

@kevindelgado
Copy link
Contributor Author

/assign @apelisse
/cc @liggitt

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 31, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Mar 31, 2021
@k8s-ci-robot k8s-ci-robot added do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 31, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @kevindelgado. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 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. area/apiserver area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Mar 31, 2021
@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 31, 2021
@liggitt
Copy link
Member

liggitt commented Mar 31, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 31, 2021
@apelisse
Copy link
Member

apelisse commented Apr 3, 2021

Not changing the way we apply the object in the test would probably be fine too, that was mostly an improvement to the test that doesn't necessarily have to be carried into 1.18.

@kevindelgado kevindelgado force-pushed the automated-cherry-pick-of-#100341-#98576-#--dry-run-upstream-release-1.18-1617135987 branch from cb3bd0c to 9709b21 Compare April 5, 2021 16:46
@kevindelgado
Copy link
Contributor Author

Sounds good, I've removed 693d0fb from this and modified the etcd testdata to set the protocol instead of getting rid of the test improvement. I will do the same for the 1.19 backport.

Once this passes CI, PTAL @apelisse @liggitt

@apelisse
Copy link
Member

apelisse commented Apr 5, 2021

LGTM, thanks!

@kevindelgado
Copy link
Contributor Author

Friendly ping @liggitt (this needs to make it by the cherry-pick deadline on friday, since it's the last release of 1.18)

@liggitt
Copy link
Member

liggitt commented Apr 6, 2021

it's a little hard to tell what is being cherry-picked and what is new in this PR now. Can you update the title/description with the master commits that are now cherry-picked, and highlight the bits that are distinct to this PR (maybe just the protocol field in the etcd stub data?)

there also look to be formatting changes in several files that are likely related to running update-gofmt with a newer version of go. revert those if possible.

@kevindelgado kevindelgado force-pushed the automated-cherry-pick-of-#100341-#98576-#--dry-run-upstream-release-1.18-1617135987 branch from 9709b21 to 56b9595 Compare April 6, 2021 23:39
@kevindelgado kevindelgado changed the title Automated cherry pick of #100341 #98576 # dry run upstream release 1.18 1617135987 cherry pick #100341 #98576 #95836 to enable SSA with APIService Apr 6, 2021
@kevindelgado
Copy link
Contributor Author

description updated, formatting updates removed, ptal @liggitt

@kevindelgado kevindelgado changed the title cherry pick #100341 #98576 #95836 to enable SSA with APIService cherry pick #100341 #98576 #95836 on 1.18 to enable SSA with APIService Apr 7, 2021
@liggitt
Copy link
Member

liggitt commented Apr 7, 2021

/retest
/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 Apr 7, 2021
@kevindelgado
Copy link
Contributor Author

ready for cherry-pick review, ping @kubernetes/release-engineering

@kevindelgado
Copy link
Contributor Author

kevindelgado commented Apr 7, 2021

@liggitt can you tag this with /approve again? I think it didn't apply the label because it was still going through CI when you first applied it.

@feiskyer
Copy link
Member

feiskyer commented Apr 8, 2021

@liggitt could you approve it again? seems the original /approve comment didn't work

@liggitt
Copy link
Member

liggitt commented Apr 8, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kevindelgado, liggitt

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 Apr 8, 2021
@kevindelgado
Copy link
Contributor Author

PR approved, ping @kubernetes/release-engineering

@feiskyer feiskyer added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. labels Apr 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit c552288 into kubernetes:release-1.18 Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/code-generation area/kubectl area/test cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants