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

chore: update the changelog to reflect the breaking change #4903

Merged
merged 2 commits into from
Sep 22, 2023

Conversation

JorTurFer
Copy link
Member

I merged a commit with the changelog entry in the wrong section

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

@JorTurFer JorTurFer requested a review from a team as a code owner August 23, 2023 19:52
@JorTurFer
Copy link
Member Author

/skip-e2e

@JorTurFer JorTurFer enabled auto-merge (squash) August 23, 2023 19:52
@github-actions
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

🏖️ Over the summer, the response time will be longer than usual due to maintainers taking time off so please bear with us.

While you are waiting, make sure to:

Learn more about:

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Jorge Turrado <[email protected]>
@tomkerkhove
Copy link
Member

I was going to say, please also refer to the GitHub Discussion but looks like we don't have any?

https://github.com/kedacore/keda/discussions/categories/deprecations

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 14, 2023

I was going to say, please also refer to the GitHub Discussion but looks like we don't have any?

kedacore/keda/discussions/categories/deprecations

This is: #4352 Should I add it also to the changelog?

@tomkerkhove
Copy link
Member

We always need to announce deprecations in dedicated discussions for awareness/visibility as per https://github.com/kedacore/governance/blob/main/DEPRECATIONS.md#introducing-new-deprecations

@JorTurFer
Copy link
Member Author

Probably we missed that part at that moment :/

@tomkerkhove
Copy link
Member

@JorTurFer not really as it's included: https://github.com/kedacore/governance/blob/main/DEPRECATIONS.md#custom-resource-definitions-crds

Custom Resource Definitions (CRDs)
Our custom resource definitions (CRDs) have a separate versioning scheme than the KEDA runtime given they have an apiVersion. For this, we follow the official Kubernetes API versioning policy.

KEDA is allowed to make breaking changes to the CRDs, when a new apiVersion is introduced.

@JorTurFer
Copy link
Member Author

@JorTurFer not really as it's included: kedacore/governance@main/DEPRECATIONS.md#custom-resource-definitions-crds

Custom Resource Definitions (CRDs)
Our custom resource definitions (CRDs) have a separate versioning scheme than the KEDA runtime given they have an apiVersion. For this, we follow the official Kubernetes API versioning policy.
KEDA is allowed to make breaking changes to the CRDs, when a new apiVersion is introduced.

This isn't a breaking change in the API because there isn't any change in the CRD. I mean, it's part of the scaler stuff, not the API itself

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 21, 2023

@JorTurFer not really as it's included: kedacore/governance@main/DEPRECATIONS.md#custom-resource-definitions-crds

Custom Resource Definitions (CRDs)
Our custom resource definitions (CRDs) have a separate versioning scheme than the KEDA runtime given they have an apiVersion. For this, we follow the official Kubernetes API versioning policy.
KEDA is allowed to make breaking changes to the CRDs, when a new apiVersion is introduced.

This isn't a breaking change in the CRD because there isn't any change in the CRD. I mean, it's part of the scaler stuff (is a change in the metadata section, that it's map[string]string, we have just deprecated and removed a key on the map), not the API itself. I meant that we missed the part of opening the discussion

@tomkerkhove
Copy link
Member

Everything that is part of the CRD is subject to that rule IMO, even if it's part of the metadata. If we get rid of it, there is impact on end-users so it has to follow the guidance

@JorTurFer
Copy link
Member Author

That's the point, it's not part of the CRD, the field that has removed isn't defined in the CRD spec.
Will this affect to users? Yes. Does this affect as if we would have changed something of the starting parameters? Yes too.
I mean, I consider a CRD change that requires api change when the CRD OpenApi changes, but this isn't the case (but maybe I'm wrong)
WDYT @zroubalik ?
We are in time to rever the change if you think too that it's not allowed by the deprecation policy

@tomkerkhove
Copy link
Member

This is where I don't agree; it's not because it's not in the CRD spec that we can just break people.

Will this affect to users? Yes. Does this affect as if we would have changed something of the starting parameters? Yes too.

This basically confirms that it's a breaking change so it has to comply in my opinion. The only reason why we don't have it in the CRD schema is because of the technical implications given metadata is freeform because of all the various trigger types.

Still, it is configuration that end-users use that we cannot break (and also why I keep saying we need #2359 :))

@JorTurFer JorTurFer merged commit 3ef38f4 into kedacore:main Sep 22, 2023
18 checks passed
@JorTurFer JorTurFer deleted the fix-changelog branch September 22, 2023 13:54
@JorTurFer
Copy link
Member Author

Wait, I didn't merge this PR. Actually IDK why it was merged O.O
@zroubalik Could you share your thoughts about if this is a breaking change or not? if you both think that it's a not allowed change, I'll revert this commit (sorry again, IDK if I merged it by error, the auto merge, or wtf) and the feature removal.

toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants