Semver lastedge error

[grokspawn]

During an onboarding session, we discovered that an author was missing an expected replaces between their last bundle version and its semver-ordered predecessor.

On investigation, it was because the channel linking step always seeks forward for a y-version change and runs off the end of the version list, and thus fails to detect the change.

When investigating, it became clear that the method was over-engineered as well, so this PR substantially simplifies the implementation.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.95%. Comparing base (efddcb3) to head (5a8f876).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1169      +/-   ##
==========================================
- Coverage   53.97%   53.95%   -0.03%     
==========================================
  Files         108      108              
  Lines       11220    11212       -8     
==========================================
- Hits         6056     6049       -7     
  Misses       4182     4182              
+ Partials      982      981       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[everettraven]

From a code standpoint, this look ok to me but I am pretty unfamiliar with this section of the code. In order to give what I feel would be a proper review I might need a more synchronous walkthrough of the changes.

[grokspawn]

From a code standpoint, this look ok to me but I am pretty unfamiliar with this section of the code. In order to give what I feel would be a proper review I might need a more synchronous walkthrough of the changes.

Happy to schedule something or grab an impromptu huddle. The short version of what happened is

1. the previous code used to keep a complete accounting of every edge, and had complex logic to identify and handle correctly changes in channel archetype, major/minor channel type, and the relevant bundle version for each edge. I basically went "this is already in unlinked channels, except for a bundle-name --> version lookup"; and
2. the previous code would miss a minor-stream increment last-1 .. last because
   1. the iterators were bound/contained by the array
   2. the yProbe looking for such iterations would have to be outside the array to detect it, which isn't allowed

so I just added a quick "did it happen" check for those edges.

[everettraven]

So after spending more time dissecting this for loop to really understand what each step is doing (took ~30min) I think it is good to go logically. Maybe it's just me coming back from being sick and still being a bit foggy (still recovering) but I felt that it was a bit difficult to understand this change in what I felt was a reasonable time frame (~20 min) as a reviewer, which led me to feeling rather unconfident in my review and thus withholding an "approval".

Feel free to take this recommendation or leave it as it, but I think a slight refactor for readability could be useful here. Maybe something along the lines of:

Suggested change

	for yProbe < entryCount {
	curVersion := bundleVersions[(*entries)[curEdge].Name]
	yProbeVersion := bundleVersions[(*entries)[yProbe].Name]
	if getMinorVersion(yProbeVersion).EQ(getMinorVersion(curVersion)) {
	yProbe += 1
	} else {
	break
	}
	}
	// if yProbe crossed a threshold, the previous entry is the last of the previous Y-stream
	preChangeIndex := yProbe - 1
	if curEdge != yProbe {
	if zmaxQueue != "" {
	// add skips edge to allow skipping over y iterations within an x stream
	(*entries)[preChangeIndex].Skips = append((*entries)[preChangeIndex].Skips, zmaxQueue)
	(*entries)[preChangeIndex].Replaces = zmaxQueue
	}
	zmaxQueue = (*entries)[preChangeIndex].Name
	}
	for curEdge < preChangeIndex {
	// add skips edges to y-1 from z < y
	(*entries)[preChangeIndex].Skips = append((*entries)[preChangeIndex].Skips, (*entries)[curEdge].Name)
	curEdge += 1
	}
	curEdge += 1
	yProbe = curEdge + 1
	}
	yStreamHead := getYStreamHead(curEntry, *entries) // This function would perform the same logic as already exists in the yProbe for loop
	if curEdge != yStreamHead {
	if zmaxQueue != "" {
	// add skips edge to allow skipping over y iterations within an x stream
	(*entries)[yStreamHead].Skips = append((*entries)[yStreamHead].Skips, zmaxQueue)
	(*entries)[yStreamHead].Replaces = zmaxQueue
	}
	zmaxQueue = (*entries)[yStreamHead].Name
	}
	// could also be made into a separate function if desired
	for _, edge := range (*entries)[curEdge:yStreamHead] {
	// add skips edges to y-1 from z < y
	(*entries)[yStreamHead].Skips = append((*entries)[yStreamHead].Skips, edge.Name)
	}
	curEdge = yStreamHead + 1
	}

[everettraven]

Overall, what threw me for a loop (pun intended) the most was wrapping my head around the multiple curEdge increments (and why) as we progressed through the loop and the initially unclear discovery of the y stream head (although looking back at it it totally makes sense as is, so feel free to keep the loop instead of breaking out to another function if you want - I just ate lunch so my mind is feeling a bit more clear now 😆 ).

[everettraven]

Have the one nit, but I don't think my concerns of readability should block this PR since functionally it is 👍 .

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, grokspawn

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:

• OWNERS [grokspawn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[everettraven]

/lgtm