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

Rename ScheduledJobs to CronJobs #36021

Merged
merged 3 commits into from
Nov 7, 2016
Merged

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Nov 1, 2016

I went with @smarterclayton idea of registering named types in schema. This way we can support both the new (CronJobs) and old (ScheduledJobs) resource name. Fixes #32150.

fyi @erictune @caesarxuchao @janetkuo

Not ready yet, but getting close there...

Release note:

Rename ScheduledJobs to CronJobs.

This change is Reviewable

@soltysh soltysh added area/batch release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 1, 2016
@soltysh
Copy link
Contributor Author

soltysh commented Nov 1, 2016

@smarterclayton after registering ScheduledJobs with in pkg/apis/batch/v2alpha1/register.go using scheme.AddKnownTypeWithName(SchemeGroupVersion.WithKind("ScheduledJob"), &CronJob{}) I'm getting failures when invoking pkg/api/defaulting_test.go. The error is pointing me as if ClusterRoleBindingList didn't need defaulting. I'm 100% positive this is caused by that AddKnownTapeWithName call, any ideas what I might be missing here? I'll add I've tried registering defaulter for ScheduledJob and that didn't help. Or maybe that defaulting_test.go doesn't handle SJ registered that way properly?

@soltysh soltysh added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 1, 2016
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 2, 2016
@smarterclayton
Copy link
Contributor

It's hard to help debug without seeing the actual failure.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2016
@soltysh
Copy link
Contributor Author

soltysh commented Nov 2, 2016

It's hard to help debug without seeing the actual failure.

nvmd, the rebase solved the problem

@soltysh
Copy link
Contributor Author

soltysh commented Nov 4, 2016

@sttts fyi

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 4483764c23de80a8542f758972d243e09937b884. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 4483764c23de80a8542f758972d243e09937b884. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 4483764c23de80a8542f758972d243e09937b884. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 4483764c23de80a8542f758972d243e09937b884. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 4483764c23de80a8542f758972d243e09937b884. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 4483764c23de80a8542f758972d243e09937b884. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@soltysh soltysh removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 4, 2016
@soltysh soltysh changed the title WIP: Rename ScheduledJobs to CronJobs Rename ScheduledJobs to CronJobs Nov 4, 2016
@soltysh soltysh added this to the v1.5 milestone Nov 4, 2016
@soltysh
Copy link
Contributor Author

soltysh commented Nov 4, 2016

This is ready for review @smarterclayton, @janetkuo, @erictune @caesarxuchao I'd like to merge it for 1.5. The current state of this PR allows transparently accessing CronJobs or ScheduledJobs. Everything is stored as CronJobs, so no matter what you're creating it'll work in future versions. Unfortunately that also means what was created previously will have to be re-created, but I guess that's a minor issue. The only problem I'm struggling right now, is that I can't get SJ to appear in the swagger, which means kubectl create needs to be run with --validate=false, but that's the only limitation I'll try to fix asap.

fyi @justinsb @chrislovecnm

@k8s-github-robot k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/old-docs labels Nov 4, 2016
@soltysh
Copy link
Contributor Author

soltysh commented Nov 4, 2016

Fixed bazel.

@smarterclayton
Copy link
Contributor

Yeah, there was a lot of code added to make this work well for minions, and it only slightly rotted.

@soltysh
Copy link
Contributor Author

soltysh commented Nov 4, 2016

Changing name looks easy. Wonder if we want to do that for the petset renaming as well.

Yup, since I know the knobs already I can volunteer to add it, if that's desirable.

@smarterclayton
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2016
@caesarxuchao
Copy link
Member

caesarxuchao commented Nov 4, 2016

Petset also changed the version from v1alpha1 to v1beta1, I bet that will complicate things. @erictune seems to be fine with abandoning petset completely.

@caesarxuchao
Copy link
Member

LGTM. Thanks.

@liggitt liggitt added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 6, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2016
@soltysh
Copy link
Contributor Author

soltysh commented Nov 7, 2016

Rebased, re-applying the label back.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit fdcdc95c3255d7c81f13956d6e4eaf78e7aafae4. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 7, 2016
@soltysh
Copy link
Contributor Author

soltysh commented Nov 7, 2016

Fixed the verification failure (codegen).

@soltysh soltysh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/batch 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. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename ScheduledJob to CronJob
7 participants