Breaking Change: Signals Handler

[ekristen]

This commit totally just changed the entire function signature for using the signals handler and there's was only a patch bump to the overall package.

I realize this is mostly used internally, but there are others that use this library, I think it would be a good idea to refrain from breaking changes especially when semver is being used but not reflected with this change.

Can we get this reverted or explain the change? It seems you can no longer send a parent context.

rancher/wrangler@8515aa0

It seems to be a more sensible course of action would have been to at least leave the existing functionality in place under a new function name or introduced the new behavior under a new function name as to not to break backwards compatibility.

[luthermonson]

The signals handler comes from upstream and they rewrote it to handle windows services so you get more access to the channel and an exported func to cancel it on demand as the signals in windows via service manager are not acknowledged. This package was always copy/pasta code from the upstream signal handler in k8s and they had updates we needed so i kept it inline with what they had instead of rewriting it all.

we used this signals update to fix rke2: rancher/rke2#2098 and is explained more in detail here: rancher/rke2#1755 with all commits to get us updated.

[ekristen]

@luthermonson thanks for the explanation and I understand the change.

I re-read your explanation a second time after writing the below, now quoted, and it didn't hit home that you said copy/paste from upstream. Either way, I think this sets a bad precedence with usage of this library to implement a breaking change regardless of source in a patch update.

While I understand the change, I disagree with the approach. It goes against all best practices in software development.

This library is used by myself (and I'm sure others) as well as MANY rancher projects, and what was implemented was a non-backwards compatible change and then released as a patch update.

This should be reverted and implemented as a second function call that has the new flow at the very minimum.

If the decision is to not do this, then at the very least this should trigger a 1.0.0 or 2.0.0 release of the library due to the breaking change.

[github-actions]

This repository uses an automated workflow to automatically label issues which have not had any activity (commit/comment/label) for 60 days. This helps us manage the community issues better. If the issue is still relevant, please add a comment to the issue so the workflow can remove the label and we know it is still valid. If it is no longer relevant (or possibly fixed in the latest release), the workflow will automatically close the issue in 14 days. Thank you for your contributions.

[ekristen]

This is still relevant

[github-actions]

This repository uses an automated workflow to automatically label issues which have not had any activity (commit/comment/label) for 60 days. This helps us manage the community issues better. If the issue is still relevant, please add a comment to the issue so the workflow can remove the label and we know it is still valid. If it is no longer relevant (or possibly fixed in the latest release), the workflow will automatically close the issue in 14 days. Thank you for your contributions.