PR#119: coreos-release-note: fix error produced when calculating pkgdiffs - fedora-web/websites

fedora-web / websites

#119 coreos-release-note: fix error produced when calculating pkgdiffs

Merged 3 years ago by dustymabe. Opened 3 years ago by abai.

fedora-web/ abai/websites fix-pkg-diff into master

coreos-release-note: fix error produced when calculating pkgdiffs

Allen Bai • 3 years ago

c3de636

sites/static/js/coreos-release-notes.js

file modified

+4 -4

		`@@ -190,10 +190,10 @@`
		`} else if (strcmp(pkgDiffAccCpy[i][2].NewPackage[1], pkgInfo.PreviousPackage[1]) > 0) {`
		`// overall, an upgrade`
		`pkgDiffAccCpyp[i] = d;`
		`- pkgDiffAccCpy[i].NewPackage[1] = dAcc[2].NewPackage[1];`
		`+ pkgDiffAccCpy[i][2].NewPackage[1] = dAcc[2].NewPackage[1];`
		`} else {`
		`// overall, a downgrade`
		`- pkgDiffAccCpy[i].PreviousPackage[1] = pkgInfo.PreviousPackage[1];`
		`+ pkgDiffAccCpy[i][2].PreviousPackage[1] = pkgInfo.PreviousPackage[1];`
		`}`
		`break;`
		`}`
		`@@ -221,11 +221,11 @@`
		`pkgDiffAccCpy[i] = [];`
		`} else if (strcmp(pkgDiffAccCpy[i][2].NewPackage[1], pkgInfo.PreviousPackage[1]) > 0) {`
		`// overall, an upgrade`
		`- pkgDiffAccCpy[i].PreviousPackage[1] = pkgInfo.PreviousPackage[1];`
		`+ pkgDiffAccCpy[i][2].PreviousPackage[1] = pkgInfo.PreviousPackage[1];`
		`} else {`
		`// overall, a downgrade`
		`pkgDiffAccCpyp[i] = d;`
		`- pkgDiffAccCpy[i].NewPackage[1] = dAcc[2].NewPackage[1];`
		`+ pkgDiffAccCpy[i][2].NewPackage[1] = dAcc[2].NewPackage[1];`
		`}`
		`break;`
		`}`

abai commented 3 years ago

This error was not found previously since the conditional block was
not hit when calculating pkgdiffs. Essentially the accumulated pkgdiff
is an array of arrays, and the previous code lacks a layer of indexing
to modify the individual pkgdiff info.

In the future, more error checkings need to be added to handle this kind
of situations more gracefully.

Signed-off-by: Allen Bai abai@redhat.com

abai commented 3 years ago

cc @dustymabe @jlebon

jlebon commented 3 years ago

I don't fully understand this code, but I trust you. :)
So LGTM!

In the future we will drop this code to something much simpler.

dustymabe commented 3 years ago

assuming this works for all current streams (stable, testing, next) this LGTM.

abai commented 3 years ago

@dustymabe I triple checked this locally to make sure no more surprises..
@jlebon yes, I was thinking this should be a temporary solution when I was writing this because it's too dependent on the structure of the metadata and a lot of corner cases to consider which makes it error prone.

EDIT: typo :)

Edited 3 years ago by abai