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

Remove deprecated field rolloutStrategy #3596

Open
JorTurFer opened this issue Aug 25, 2022 · 12 comments
Open

Remove deprecated field rolloutStrategy #3596

JorTurFer opened this issue Aug 25, 2022 · 12 comments
Labels
breaking-change good first issue Good for newcomers stale-bot-ignore All issues that should not be automatically closed by our stale bot

Comments

@JorTurFer
Copy link
Member

Proposal

In #3338 we introduced a new section that extends this.
We need to remove the deprecated rolloutStrategy

Use-Case

No response

Anything else?

No response

@JorTurFer JorTurFer added needs-discussion feature-request All issues for new features that have not been committed to breaking-change stale-bot-ignore All issues that should not be automatically closed by our stale bot labels Aug 25, 2022
@tomkerkhove tomkerkhove removed feature-request All issues for new features that have not been committed to needs-discussion labels Aug 25, 2022
@Ritikaa96
Copy link
Contributor

Hi, @JorTurFer , I'd like to take this up. What I understood is we have to remove rolloutStartegy from :

  • apis/keda/v1alpha1/scaledjob_types.go
  • controllers/keda/scaledjob_controller.go
  • config/crd/bases/keda.sh_scaledjobs.yaml

There is no mention of rolloutStrategy anywhere else so we are safe to deprecate it through these.

@tomkerkhove
Copy link
Member

Thanks! Unfortunately we cannot do this yet as this is a breaking change and we have to wait for KEDA 3.X.

@Ritikaa96
Copy link
Contributor

Thanks! Unfortunately we cannot do this yet as this is a breaking change and we have to wait for KEDA 3.X.

oh! alright! seems like a rational approach ! thanks for the quick reply :)

@tomkerkhove
Copy link
Member

No problem and thanks for looking for contribution items!

@zroubalik
Copy link
Member

Honestly I think we can drop it :D it's change in crd which is v1alpha1.

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 23, 2022

IMO, we should keep at least 1 version the old code with a warning to avoid breaking changes, giving some time (2-3 months) to update the manifest. Something like, WARNING: the field XXX has been deprecated in favor of YYY and will be removed in vZZZZ(like k8s does)

@JorTurFer
Copy link
Member Author

I mean, I agree with removing the fields, but giving info to the users and time to update, we could improve the user experience. In the case of we notify it and they don't update their manifest, it's totally their fault.
Anyway, we should document this also in kedacore/governance#70

@zroubalik
Copy link
Member

Absolutely, this is the way.

@tomkerkhove
Copy link
Member

Which is a good reminder to ask to re-review open comments on kedacore/governance#70 :)

@JorTurFer
Copy link
Member Author

Based on our deprecation policies, I guess this should be removed in v2.10

@JorTurFer JorTurFer added the good first issue Good for newcomers label Dec 9, 2022
@tomkerkhove
Copy link
Member

@JorTurFer No, it needs a new API version to remove it as it's part of our CRD AFAIK

@JorTurFer
Copy link
Member Author

true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change good first issue Good for newcomers stale-bot-ignore All issues that should not be automatically closed by our stale bot
Projects
Status: Proposed
Development

Successfully merging a pull request may close this issue.

4 participants