Remove special handling of targetpath for deprecated Kubernetes 1.24

[psy-q]

This handling is no longer necessary as Kuberenetes 1.24 is not supported anymore.

Additionally, the version comparison that was used will break on some Kubernetes versions (any SemVer with a non-integer component). Removing the check fixes this problem as well.

Closes #623

[iPraveenParihar]

Thanks @psy-q, Can you please squash your commits into one and add commit sign-off?

[psy-q]

Thanks, I've squashed the commits now. Sorry for the noisy PR, it's my first contribution in Go. If there's something I've missed, please let me know.

[psy-q]

I've spent some time yesterday trying to write tests for this but:

• Running the code in question requires a ReclaimSpaceServer
  • That needs a fake Kubernetes server, but I'm not sure fake can fake different version responses
  • The server needs a fake gRPC client, I've tried just mocking the Connect() and Close() functions but didn't get far
• It needs a mocked PVC to pass to getStagingTargetPath() -- depending on how convincing the mock needs to be, this is probably the easiest part with some code from pvc_test.go

I'm out of my depth here and not getting any results but I'd be willing to work on a test a bit more if there's some easier way to get a ReclaimSpaceServer.

[nixpanic]

This looks good to me, thanks!

If you want to test this out, the easiest is to introduce a function like

func needsLegacyGlobalmount(serverVersion string) bool {
    semanticVersion := version.MustParseSemantic(serverVersion)
    legacyVersion := version.MustParseSemantic("v1.24.0")

    return semanticVersion.LessThan(legacyVersion)
}

This then makes it possible to test with different versions in reclaimspace_test.go.

[Rakshith-R]

missed madhu's comment regarding 1.24's deprecation

[Rakshith-R]

#625 (comment)

Sorry again i missed adding one point, we can remove this entire check as we don't need to support 1.24 anymore as its a deprecated or unsupported Kubernetes version. IMO we can remove all these checks

The discussion was marked resolved and therefore not visible in the thread.
I think we can remove the check entirely too.

[psy-q]

I've noticed this error checking is now pointless since it's always nil.

Should I remove the check as well with this PR or leave that part as it is?

[Rakshith-R]

I've noticed this error checking is now pointless since it's always nil.

Should I remove the check as well with this PR or leave that part as it is?

Sure, you can remove it too in this PR.

[psy-q]

Sorry, I never worked with GitHub reviews: It still shows one requested change for me, I tried to close that as the code in question no longer exists but I suppose it's @Rakshith-R and not me who can mark the review as resolved?

[nixpanic]

Sorry, I never worked with GitHub reviews: It still shows one requested change for me, I tried to close that as the code in question no longer exists but I suppose it's @Rakshith-R and not me who can mark the review as resolved?

Yes, that is correct. @Rakshith-R requested a change and he needs to confirm it was addressed. Maintainers of the repository can remove that requirement too, but we only do that for exceptional cases.

[Rakshith-R]

Thanks

[Rakshith-R]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased