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

Remove deprecated files and functions #6730

Merged
merged 1 commit into from Feb 8, 2024
Merged

Remove deprecated files and functions #6730

merged 1 commit into from Feb 8, 2024

Conversation

inteon
Copy link
Member

@inteon inteon commented Feb 8, 2024 •
edited

Removes functions and files that are deprecated, goal is to reduce clutter in codebase.

Kind

/kind cleanup

Release Note

Removed deprecated util functions that have been replaced by the `slices` and `k8s.io/apimachinery/pkg/util` packages.
Removed deprecated CSR functions which have been replaced with other functions in the `pkg/util/pki` package.

@jetstack-bot jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/deploy Indicates a PR modifies deployment configuration area/testing Issues relating to testing labels Feb 8, 2024
@inteon
remove deprecated files and functions
0422044 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
wallrj
wallrj approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks Tim.

I was also annoyed by all the lingering bazel files, so I'm glad to see them removed.

Also happy to see the removal of the deprecated Go functions, but notice that we're quite inconsistent about how long (and how many minor releases) we wait before removing them.

Please update the release note so explain which deprecated modules and functions have been removed.

/hold in case you were not ready for this to be reviewed.

/lgtm

internal/cmd/util/defaults.go Show resolved Hide resolved
pkg/util/pki/certificatetemplate.go
@@ -377,38 +377,3 @@ func CertificateTemplateFromCertificateSigningRequest(csr *certificatesv1.Certif
CertificateTemplateValidateAndOverrideKeyUsages(ku, eku), // Override the key usages, but make sure they match the usages in the CSR if present
)
}

Copy link
Member

Choose a reason for hiding this comment

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

These functions were deprecated since v1.13.0 which was released ~4 months ago on Sep 12, 2023.

I think that's probably enough notice and in the README file we say:

cert-manager/README.md

Lines 84 to 86 in b656118

⚠️ Please note that cert-manager **does not** currently provide a Go module compatibility guarantee. That means that
**most code under `pkg/` is subject to change in a breaking way**, even between minor or patch releases and even if
the code is currently publicly exported.

pkg/util/util.go
// Deprecated: use slices#Equal instead
func EqualSorted(s1, s2 []string) bool {
return slices.Equal(s1, s2)
}
Copy link
Member

Choose a reason for hiding this comment

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

You deprecated these in #6593 which was released in v1.14.0 this month:

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be very surprised if someone imported these functions.
So I think it is fine to remove them this soon.

@jetstack-bot jetstack-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 8, 2024
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 8, 2024
@inteon
Copy link
Member Author

inteon commented Feb 8, 2024 •
edited

/unhold
I updated the release notes.

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2024
@inteon
Copy link
Member Author

inteon commented Feb 8, 2024

/retest

@jetstack-bot jetstack-bot merged commit 63bd488 into cert-manager:master Feb 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wallrj wallrj wallrj approved these changes

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/deploy Indicates a PR modifies deployment configuration area/testing Issues relating to testing dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@inteon @jetstack-bot @wallrj