community icon indicating copy to clipboard operation
community copied to clipboard

Unhandled case in `equalStrings` function implementation

Open a-hilaly opened this issue 2 years ago • 5 comments

The equalStrings function in most of (if not all) hooks.go files compares two string pointers and returns whether they are equal or not while taking into consideration that a nil pointer and an empty string are equal. However, there is a case that is not being handled correctly in the current implementation. As a result, the function may not return the expected result Here is the implementation:

func equalStrings(a, b *string) bool {
	if a == nil {
		return b == nil || *b == ""
	}
	return (*a == "" && b == nil) || *a == *b
}

Again thank you @embano1 for spotting this!

a-hilaly avatar Feb 01 '23 09:02 a-hilaly

However, there is a case that is not being handled correctly in the current implementation.

What case is not being handled?

jaypipes avatar Feb 01 '23 15:02 jaypipes

What case is not being handled?

*a != "" && b == nil. Correct implementation should be something like:

func equalStrings(a, b *string) bool {
	if a == nil {
		return b == nil
	} else if b == nil {
		return false
	}
	return *a == *b
}

a-hilaly avatar Feb 01 '23 15:02 a-hilaly

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Provide feedback via https://github.com/aws-controllers-k8s/community. /lifecycle stale

ack-bot avatar May 02 '23 20:05 ack-bot

Issues go stale after 180d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 60d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Provide feedback via https://github.com/aws-controllers-k8s/community. /lifecycle stale

ack-bot avatar Nov 20 '23 23:11 ack-bot

Issues go stale after 180d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 60d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Provide feedback via https://github.com/aws-controllers-k8s/community. /lifecycle stale

ack-bot avatar May 19 '24 01:05 ack-bot

Issues go stale after 180d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 60d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Provide feedback via https://github.com/aws-controllers-k8s/community. /lifecycle stale

ack-bot avatar Dec 01 '24 12:12 ack-bot

Stale issues rot after 60d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 60d of inactivity. If this issue is safe to close now please do so with /close. Provide feedback via https://github.com/aws-controllers-k8s/community. /lifecycle rotten

ack-bot avatar Jan 30 '25 12:01 ack-bot

Rotten issues close after 60d of inactivity. Reopen the issue with /reopen. Provide feedback via https://github.com/aws-controllers-k8s/community. /close

ack-bot avatar Mar 31 '25 13:03 ack-bot

@ack-bot: Closing this issue.

In response to this:

Rotten issues close after 60d of inactivity. Reopen the issue with /reopen. Provide feedback via https://github.com/aws-controllers-k8s/community. /close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ack-prow[bot] avatar Mar 31 '25 13:03 ack-prow[bot]