Skip to content

Conversation

@lsm5
Copy link
Member

@lsm5 lsm5 commented Nov 7, 2025

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

  • Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
    commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
    the sign-off email address. See CONTRIBUTING.md
    for more information.
  • Referenced issues using Fixes: #00000 in commit message (if applicable)
  • Tests have been added/updated (or no tests are needed)
  • Documentation has been updated (or no documentation changes are needed)
  • All commits pass make validatepr (format/lint checks)
  • Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

None

lsm5 added 2 commits November 7, 2025 15:17
Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Obsoletes: containers#27407

Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
@lsm5 lsm5 added the No New Tests Allow PR to proceed without adding regression tests label Nov 7, 2025
@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels Nov 7, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2025
@mheon
Copy link
Member

mheon commented Nov 9, 2025

Sure, LGTM

@lsm5
Copy link
Member Author

lsm5 commented Nov 10, 2025

This exposed more deprecations. Working through those now.

@lsm5 lsm5 changed the title Deprecation notice update golangci-lint bump and deprecation cleanups Nov 10, 2025
@lsm5 lsm5 marked this pull request as ready for review November 10, 2025 17:10
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2025
@lsm5
Copy link
Member Author

lsm5 commented Nov 10, 2025

@containers/podman-maintainers PTAL.

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

This is a drive-by, I didn’t actually look at any of the removed fields and I have, currently, no opinion on whether removing the uses is or isn’t correct.

// check if it is prefaced with container.seccomp.security.alpha.kubernetes.io/
prefixAndCtr := strings.Split(annKeyValue, "/")
if prefixAndCtr[0]+"/" != v1.SeccompContainerAnnotationKeyPrefix {
if prefixAndCtr[0]+"/" != "container.seccomp.security.alpha.kubernetes.io/" {
Copy link
Contributor

Choose a reason for hiding this comment

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

These kinds of changes are making things harder to maintain.

If the code needs to stay (I have absolutely no expertise here and no opinion on that), it’s better to use the named constants, and silence the linter with an explanation why the code still needs to stay.

This continues to use a deprecated feature, but it only makes it impossible to find references in any future analysis.

if c.config.Rootfs != "" {
size, err := util.SizeOfPath(c.config.Rootfs)
return int64(size), err
size, err := directory.Size(c.config.Rootfs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn’t this the last user? The function can be removed, then.

// check if it is prefaced with container.seccomp.security.alpha.kubernetes.io/
prefixAndCtr := strings.Split(annKeyValue, "/")
if prefixAndCtr[0]+"/" != v1.SeccompContainerAnnotationKeyPrefix {
if prefixAndCtr[0]+"/" != "container.seccomp.security.alpha.kubernetes.io/" {
Copy link
Member

Choose a reason for hiding this comment

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

That is just objectivity worse, what do we gain by inline this and keep the deprecated variable as well? If these annotations are no longer in use can we rework the seccomp logic to no longer use them instead?

same in the other cases here

// if no network was specified use add the default
if len(s.Networks) == 0 {
// backwards config still allow the old cni networks list and convert to new format
if len(s.CNINetworks) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

logic wise it would be nice to do this and the other cni removal in the same commit instead of splitting the commits per file which doesn't seem to gain us anything here.

This seems safe since we drop cni support but if we do this then also directly remove the CNINetworks field from the struct definition. IF we own the code no point in having unused fields.

// Optional.
//
// Deprecated: as of podman 4.0 use "Networks" instead.
CNINetworks []string `json:"cni_networks,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

if we remove CNI support just remove the fields completely

Comment on lines 1203 to 1205
//
// Deprecated: use github.com/containers/storage/pkg/directory.Size() instead.
func SizeOfPath(path string) (uint64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

IF you remove the one caller just remove this function from the code

ctrConfig.Networks[net] = opts
}
ctrConfig.StaticIP = nil
ctrConfig.StaticMAC = nil
Copy link
Member

Choose a reason for hiding this comment

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

there are old fields, I don't think they have been in use since sqlite so they also should be removed from the struct

@lsm5
Copy link
Member Author

lsm5 commented Nov 11, 2025

@Luap99 I'm not familiar with the seccomp stuff and I'd prefer not to take too long on this PR, so would you be ok if I reverted the seccomp stuff along with nolint and FIXME notes added? The other comments seems manageable.

If not, maybe I'll just close this and/or let someone else pick up the golangci version update.

@Luap99
Copy link
Member

Luap99 commented Nov 11, 2025

I am totally fine with nolint comments on the seccomp things, I also have no idea what the expected k8s behaviours are there. Maybe file an issue for that then so we don't loose track.

lsm5 added 4 commits November 11, 2025 09:31
Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
- Remove deprecated util.SizeOfPath

Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Ref: containers#27501

Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
@lsm5 lsm5 force-pushed the deprecation-notice-update branch from 4ea376c to 43e52f7 Compare November 11, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. No New Tests Allow PR to proceed without adding regression tests release-note-none

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants