Skip to content

Use stdlib function instead of own to check for slice membership#9290

Closed
ycombinator wants to merge 1 commit intoelastic:mainfrom
ycombinator:version-membership-use-stdlib
Closed

Use stdlib function instead of own to check for slice membership#9290
ycombinator wants to merge 1 commit intoelastic:mainfrom
ycombinator:version-membership-use-stdlib

Conversation

@ycombinator
Copy link
Contributor

Quick cleanup on the heels of #9266. Thought of this right after I merged that PR 🤦.

@ycombinator ycombinator requested a review from a team as a code owner August 7, 2025 22:24
@ycombinator ycombinator added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Aug 7, 2025
@ycombinator ycombinator added skip-changelog backport-8.19 Automated backport to the 8.19 branch backport-9.1 Automated backport to the 9.1 branch labels Aug 7, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

cc @ycombinator

Copy link
Contributor

@pkoutsovasilis pkoutsovasilis left a comment

Choose a reason for hiding this comment

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

Tbh I reviewed the original PR and I thought that you went the manual approach becuase slices.Contains does essentially the following

package slices
 
// Contains reports whether v is present in s.
func Contains[S ~[]E, E comparable](s S, v E) bool {
	return Index(s, v) >= 0
}

// Index returns the index of the first occurrence of v in s,
// or -1 if not present.
func Index[S ~[]E, E comparable](s S, v E) int {
	for i := range s {
		if v == s[i] {
			return i
		}
	}
	return -1
}

and because you pass pointers this essentially is not checking the version equality but the ptr ref, what am I missing?

Also I see in the CI that adding the label doesn't invoke the step which is really weird

@ycombinator
Copy link
Contributor Author

and because you pass pointers this essentially is not checking the version equality but the ptr ref, what am I missing?

Yeah, good point. I could do something like this:

for _, ver := range versions {
	if slices.ContainsFunc(echVersions, func(candidateVer *version.ParsedSemVer) bool {
		return candidateVer.Equal(*ver)
	}) {
		filteredVersions = append(filteredVersions, ver)
	}
}

But I'm not sure that's buying us much over the current manual approach. WDYT?

Also I see in the CI that adding the label doesn't invoke the step which is really weird

In fairness, I added the label after the CI run had already started. Let me rebuild now and see if the label takes effect.

@elastic-sonarqube
Copy link

@pkoutsovasilis
Copy link
Contributor

Yeah, good point. I could do something like this:

for _, ver := range versions {
	if slices.ContainsFunc(echVersions, func(candidateVer *version.ParsedSemVer) bool {
		return candidateVer.Equal(*ver)
	}) {
		filteredVersions = append(filteredVersions, ver)
	}
}

But I'm not sure that's buying us much over the current manual approach. WDYT?

It feels like the same approach that is already there 🙂

Also I see in the CI that adding the label doesn't invoke the step which is really weird

In fairness, I added the label after the CI run had already started. Let me rebuild now and see if the label takes effect.

I cannot wrap my head around how BK reacts to the gh labels, because the test is still not invoked

@ycombinator
Copy link
Contributor Author

It feels like the same approach that is already there 🙂

Agreed. Let's just stick with what we already have. Closing this PR unmerged.

I cannot wrap my head around how BK reacts to the gh labels, because the test is still not invoked

Same. Let's use #9286 to fix the automation for this label.

@ycombinator ycombinator closed this Aug 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-8.19 Automated backport to the 8.19 branch backport-9.1 Automated backport to the 9.1 branch skip-changelog Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Testing:run:TestUpgradeIntegrationsServer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants