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 ControllerRevision to apps/v1beta2 #50698

Merged
merged 2 commits into from
Aug 17, 2017
Merged

Add ControllerRevision to apps/v1beta2 #50698

merged 2 commits into from
Aug 17, 2017

Conversation

liyinan926
Copy link
Contributor

What this PR does / why we need it:
This PR added ControllerRevision currently in apps/v1beta1 to apps/v1beta2.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #50696.

Special notes for your reviewer:
@kow3ns @janetkuo

Release note:

Add ControllerRevision to apps/v1beta2

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 15, 2017
@k8s-ci-robot
Copy link
Contributor

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

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 15, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 15, 2017
@foxish
Copy link
Contributor

foxish commented Aug 15, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 15, 2017
@yujuhong yujuhong assigned kow3ns and janetkuo and unassigned yujuhong Aug 15, 2017
dixudx
dixudx reviewed Aug 16, 2017
View reviewed changes
pkg/apis/apps/v1beta2/zz_generated.conversion.go
@@ -41,6 +41,10 @@ func init() {
// Public to allow building arbitrary schemes.
func RegisterConversions(scheme *runtime.Scheme) error {
return scheme.AddGeneratedConversionFuncs(
Convert_v1beta2_ControllerRevision_To_apps_ControllerRevision,
Copy link
Member

@dixudx dixudx Aug 16, 2017 •
edited

Choose a reason for hiding this comment

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

@liyinan926 We'd better separate such auto-generated files in another commit, which will be easy to get rebased and reviewed.

ping @janetkuo, PTAL #49645 for conversion tests. For this new added ControllerRevision, shall we append the tests in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split the auto-generated changes out into a separate commit.

@liyinan926 liyinan926 force-pushed the cr-v1beta2 branch 2 times, most recently from 13617be to 27e3cdf Compare August 16, 2017 05:41
@kow3ns
Copy link
Member

kow3ns commented Aug 16, 2017

/retest

@liyinan926
Copy link
Contributor Author

@kow3ns for some reason, the autogen didn't make changes to the swagger/open-api specs and API reference docs. Is it because appsv1/beta2 has not been enabled yet (#50643)?

kow3ns
kow3ns reviewed Aug 16, 2017
View reviewed changes
Copy link
Member

@kow3ns kow3ns left a comment

Choose a reason for hiding this comment

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

In general looks good but you need to add the registration here

@liyinan926
Copy link
Contributor Author

@kow3ns added the registration.

kow3ns
kow3ns reviewed Aug 16, 2017
View reviewed changes
test/integration/etcd/etcd_storage_path_test.go
@@ -168,6 +168,10 @@ var etcdStorageData = map[schema.GroupVersionResource]struct {
expectedEtcdPath: "/registry/replicasets/etcdstoragepathtestnamespace/rs2",
expectedGVK: gvkP("extensions", "v1beta1", "ReplicaSet"),
},
gvr("apps", "v1beta2", "controllerrevisions"): {
stub: `{"metadata":{"name":"crs2"},"data":{"name":"abc","namespace":"default","creationTimestamp":null,"Spec":{"Replicas":0,"Selector":{"matchLabels":{"foo":"bar"}},"Template":{"creationTimestamp":null,"labels":{"foo":"bar"},"Spec":{"Volumes":null,"InitContainers":null,"Containers":null,"RestartPolicy":"Always","TerminationGracePeriodSeconds":null,"ActiveDeadlineSeconds":null,"DNSPolicy":"ClusterFirst","NodeSelector":null,"ServiceAccountName":"","AutomountServiceAccountToken":null,"NodeName":"","SecurityContext":null,"ImagePullSecrets":null,"Hostname":"","Subdomain":"","Affinity":null,"SchedulerName":"","Tolerations":null,"HostAliases":null}},"VolumeClaimTemplates":null,"ServiceName":""},"Status":{"ObservedGeneration":null,"Replicas":0}},"revision":0}`,
expectedEtcdPath: "/registry/controllerrevisions/etcdstoragepathtestnamespace/crs2",
Copy link
Member

Choose a reason for hiding this comment

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

You need to update the expectedGVK (GroupVersionKind) to be gvkP("apps", "v1beta1", "ControllerRevision") because the storage format has not been migrated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kow3ns
Copy link
Member

kow3ns commented Aug 16, 2017

@liyinan926 You are probably going to want to add your update commit on top of your initial, or just squash it into the initial as you'll have to do that prior to merging, and add your code generation on top of that. The missing portion of the swagger spec is very likely due to the lack of registration of the group/version/kind as you suggest above.

@liyinan926 liyinan926 force-pushed the cr-v1beta2 branch 2 times, most recently from e760a1e to e7eda01 Compare August 16, 2017 22:34
@liyinan926
Copy link
Contributor Author

@kow3ns re-arranged the commits. Interestingly, after rebasing from the master, autogen touched the swagger/openapi specs and api references. PTAL. Thanks!

@kow3ns
Copy link
Member

kow3ns commented Aug 16, 2017

@liyinan926 The autogen creates the swagger spec from a running instance of the API server. As the apps/v1beta2/ControllerRevision group version kind was not registered during your first run, the documentation was not generated.
In general LGTM, you need to rebase and squash your pull.

@liyinan926
Copy link
Contributor Author

@kow3ns you meant re-arranging commits such that changes are made on top of latest master? Or were you suggesting merging the API changes and auto-generated stuffs together?

@liyinan926 liyinan926 force-pushed the cr-v1beta2 branch 2 times, most recently from e2eb7a8 to a92c620 Compare August 17, 2017 00:50
@liyinan926
Copy link
Contributor Author

@kow3ns rebased on master and re-arranged commits. PTAL. Thanks!

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2017
@liyinan926
Copy link
Contributor Author

/retest

@liyinan926
Copy link
Contributor Author

/assign @lavalamp

@kow3ns
Copy link
Member

kow3ns commented Aug 17, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2017
@liyinan926
Copy link
Contributor Author

liyinan926 commented Aug 17, 2017 •
edited

/unassign @lavalamp @janetkuo @dchen1107

@smarterclayton
Copy link
Contributor

/approve

@liyinan926
Copy link
Contributor Author

/assign @smarterclayton Need your approval. PTAL. Thanks!

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kow3ns, liyinan926, smarterclayton

Associated issue: 50696

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 50563, 50698, 50796)

@k8s-github-robot k8s-github-robot merged commit 9d302ec into kubernetes:master Aug 17, 2017
@liyinan926 liyinan926 deleted the cr-v1beta2 branch August 17, 2017 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dixudx dixudx dixudx left review comments

@kow3ns kow3ns kow3ns left review comments

Assignees

@kow3ns kow3ns

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ControllerRevision to apps/v1beta2