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

fix(#3393): Update to CronJob batch/v1 #3402

Merged
merged 2 commits into from
Jul 1, 2022

Conversation

christophd
Copy link
Contributor

@christophd christophd commented Jun 28, 2022

CronJob batch/v1beta1 is deprecated and will be removed soon. Updating to batch/v1 API

Fixes #3393
Fixes #2408

Release Note

Chore(deps): Update CronJob from batch/v1beta1 to batch/v1

CronJob batch/v1beta1 is deprecated and will be removed soon. Updating to batch/v1 API
@christophd
Copy link
Contributor Author

@squakez maybe respin the failed openshift workflow job?

@astefanutti
Copy link
Member

I doubt the batch/v1 group is available on OpenShift 3, which is used in the OpenShift workflow. One strategy would be to make the CronJob API optional, which would be useful beyond CI.

@lburgazzoli
Copy link
Contributor

I doubt the batch/v1 group is available on OpenShift 3, which is used in the OpenShift workflow. One strategy would be to make the CronJob API optional, which would be useful beyond CI.

+1 for making it optional

beside that, should we really keep support for OpenShift 3

@christophd
Copy link
Contributor Author

@astefanutti oh, I was not even looking at the failed workflow job error and thought it to be a flaky test run. many thanks!

let me see what it takes to make this optional. I think it is not only the trait that is using the CronJob API. I saw references to this all over the place.

@tadayosi
Copy link
Member

beside that, should we really keep support for OpenShift 3

I think we are ready to remove OpenShift 3 support. The workflow is kept only because right now oc cluster up is the only way to test the OpenShift aspects of the features on GitHub. We'll not be able to have an alternative until MicroShift becomes usable.

oh, I was not even looking at the failed workflow job error and thought it to be a flaky test run. many thanks!

A short while ago we restored stability in our test suite. Now there shouldn't be so many flaky tests, so let's keep an eye on it and try not to introduce more failing tests.

let me see what it takes to make this optional. I think it is not only the trait that is using the CronJob API. I saw references to this all over the place.

We can also disable E2E tests for CronJob when they run on OpenShift 3, just like this:
https://github.com/apache/camel-k/blob/main/e2e/common/cli/install_test.go#L101-L107

@squakez
Copy link
Contributor

squakez commented Jun 29, 2022

Skipping the test won't solve the problem happening at runtime in Openshift 3. If we want to support OC3 then we need a different strategy (ie, creating a CronJobStrategy type and using the old dependencies when we detect OC3, or new dependencies otherwise). However it's too much hassle in my opinion. Any reason why we don't want to drop OC3 support and make it easier?

@christophd christophd force-pushed the issue/3393/cronjob-batch-v1 branch from 1b0b0cb to bfc512f Compare June 29, 2022 07:34
@tadayosi
Copy link
Member

Any reason why we don't want to drop OC3 support and make it easier?

I'm fine with dropping OC3 support. It's just that if we remove the openshift workflow we lose a way to validate OpenShift related features such as S2I builds or Route easily in our daily CI.

@christophd
Copy link
Contributor Author

I think the only reason to keep OpenShift 3.x support is for developers (and our GitHub actions CI) running on Minishift

@tadayosi
Copy link
Member

I think the only reason to keep OpenShift 3.x support is for developers (and our GitHub actions CI) running on Minishift

We dropped Minishift support a while ago. What's running in the openshift workflow is the oc cluster up from OCP 3, which was another way to run a OCP cluster locally.

@heiko-braun
Copy link

@christophd can we get this into 1.8.x and 1.9.x too?

@christophd christophd force-pushed the issue/3393/cronjob-batch-v1 branch 5 times, most recently from d4dfc48 to 958c23d Compare June 30, 2022 07:22
pkg/cmd/operator/operator.go Outdated Show resolved Hide resolved
pkg/trait/container.go Outdated Show resolved Hide resolved
pkg/trait/mount.go Outdated Show resolved Hide resolved
e2e/support/test_support.go Outdated Show resolved Hide resolved
- Explicitly check for CronJob batch/v1 API availability before using it
- CronJob batch/v1 API is not available on OpenShift 3.x
- Disable cron job e2e test on OpenShift 3.x
@christophd christophd force-pushed the issue/3393/cronjob-batch-v1 branch from 958c23d to 9e6d3f4 Compare June 30, 2022 08:26
Copy link
Member

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

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

LTGM!

@christophd
Copy link
Contributor Author

@lburgazzoli @squakez @tadayosi we finally managed to make the CronJob functionality optional and only use it when the batch/v1 API is available. The e2e test is disabled when running on OpenShift 3.x

Are we good to merge?

@lburgazzoli
Copy link
Contributor

fine with me

Copy link
Member

@tadayosi tadayosi left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@tadayosi tadayosi merged commit ee05ecc into apache:main Jul 1, 2022
@tadayosi
Copy link
Member

tadayosi commented Jul 1, 2022

@christophd While this still passes CI there is an assertion failure here:
https://github.com/apache/camel-k/runs/7144173942?check_suite_focus=true#step:7:23
Can you have a look?

@christophd
Copy link
Contributor Author

@tadayosi nice spot! Many thanks! Fixed with #3411

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from batch/v1beta1 to batch/v1 Migrate CronJob to batch/v1
6 participants