Add mixin version when building the bundle #1952

joshuabezaleel · 2022-03-01T15:44:28Z

What does this change

Append the version of the mixin when building the bundle.

What issue does it fix

Closes #1948

Notes for the reviewer

Checklist

Did you write tests?
Did you write documentation?
Did you change porter.yaml or a storage document record? Update the corresponding schema file.
If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

Comment with /azp run test-porter-release if a magefile or build script was modified
Comment with /azp run porter-integration if it's a non-trivial PR

joshuabezaleel · 2022-03-01T15:49:49Z

1948-bundle-mixin-version.mp4

There are still some failing tests, perhaps because the bit to the pkg/porter/build.go hasn't been added yet. Will address it tomorrow.

carolynvs · 2022-03-01T18:35:25Z

Oh I'm so glad that you picked this up, thanks! 👍

carolynvs

This looks right, maybe for the tests try running mage UpdateTestFiles and see if they start passing?

carolynvs · 2022-03-28T16:24:07Z

@joshuabezaleel Let me know if you'd like help getting the build passing.

joshuabezaleel · 2022-04-13T03:58:37Z

I ran mage UpdateTestFiles but the test still fails.
There's one suspicion that I have yet to confirm and find a way to resolve.

I tried to print the ManifestConverter and its generated stamp and try to compare its different output between the one at TestConfig_GenerateStamp on pkg/cnab/config-adapter/stamp_test.go and to one running porter build command using the whalesay bundle.

The Mixins part both on ManifestConverter and stamp was empty on the TestConfig_GenerateStamp test compared to the porter build output which I suspect was caused by the mixins' metadata on invoking NewManifestConverter was supplied by nil (the 4th input parameter), compared to the one at the build process which was supplied by the output of getUsedMixins().

getUsedMixins() was Porter's struct method so I feel conflicted if I need to instantiate a NewTestPorter at pkg/cnab/config-adapter/stamp_test.go since it should only test things related to package pkg/cnab.
I just remembered that you mentioned on the issue that the getUsedMixins function should be changed as well, but the bundles' mixins' version is already on the bundle.json just by the change on the /pkg/cnab/config-adapter/stamp.go as was shown on the video I've attached earlier. What should be changed on this getUsedMixins, if I may ask?

So sorry that it took me a really long time to reply to this one, Carolyn. Looking forward to hearing from you.

carolynvs

Heads up that one of the tests you are working with has changed on the release/v1 branch. You'll need to merge with the most recent changes in release/v1. Let me know if you have trouble and I can help fix the merge conflicts.

carolynvs · 2022-04-19T14:11:21Z

pkg/cnab/config-adapter/stamp.go

-	stamp.Mixins = make(map[string]MixinRecord, len(c.Manifest.Mixins))
-	for _, m := range c.Manifest.Mixins {
-		stamp.Mixins[m.Name] = MixinRecord{}
+	stamp.Mixins = make(map[string]MixinRecord, len(c.Mixins))


Here you are looping over the list of installed mixins, not the list of mixins used in the bundle. You need to loop over c.Manifest.Mixins which are the mixins used by the bundle. Then match the mixin name against the list of installed mixins c.Mixins to lookup the version number.

Feel free to rename c.Mixins to c.InstalledMixins to help clear up confusion between those two variables.

The other problem is that the existing unit tests that test this functionality are not passing in a list of installed mixins. You'll need to change the tests like so:

installedMixins := []mixin.Metadata{ {Name: "exec", VersionInfo: pkgmgmt.VersionInfo{Version: "v1.2.3"}}, } a := NewManifestConverter(c.Context, m, nil, installedMixins)

Once you are done, you either need to update the test files with mage updateTestFiles or edit pkg/cnab/config-adapter/testdata/mybuns.bundle.json by hand so that it includes the mixin version number.

Ah got it, I was confused first what was the difference between c.Manifest.Mixins and c.Mixins as they contains the same list of mixins but the latter contains the version info while the former doesn't, so I just simply grab the latter one. Now it just makes sense to me that we need to iterate over the mixins defined in the manifest first.

I tried to resolve the conflict and fix the unit test, hopefully it already pass. Definitely couldn't do any contributions without your kind guidance and thoughtful review, Carolyn. Thank you lots! 🙂

carolynvs · 2022-04-27T13:16:59Z

pkg/cnab/config-adapter/stamp.go

@@ -109,7 +118,7 @@ func (c *ManifestConverter) DigestManifest() (string, error) {
 	v := pkg.Version
 	data = append(data, v...)

-	for _, m := range c.Mixins {
+	for _, m := range c.InstalledMixins {


I think this may be a bug here (not caused by your change here).

The digest of the manifest is supposed to represent the mixins used by the bundle. But this loop is adding all installed mixins into the digest which is incorrect. Installed mixins has all mixins installed, including those not used by the current bundle.

Can you make a function out of that for loop above that finds the version of only mixins that are used by the bundle, and we can reuse that function here as well to fix the bug.

Updated 😄

carolynvs · 2022-04-27T13:19:45Z

pkg/cnab/config-adapter/testdata/mybuns.bundle.json

-      "mixins": {
-        "exec": {}
-      },
+      "mixins": {},


This doesn't have any mixins listed, along with their version (which is the point of the PR). Can you identify which test goes with this file and modify it so that the list of installed mixins is populated. This file should look like this when you have it right:

"mixins": {"exec" {"version": "v1.2.3"}}

carolynvs

I left some comments with a few more changes we need to make before this is ready

Signed-off-by: joshuabezaleel <[email protected]>

- Compare mixins installed with the ones defined in the manifest and then retrieved the version info - Update test to include mixin's version Signed-off-by: joshuabezaleel <[email protected]>

Signed-off-by: joshuabezaleel <[email protected]>

carolynvs · 2022-04-27T19:12:19Z

@joshuabezaleel Just a general request for pull requests going forward, when possible do not use rebase after someone has started reviewing it.

When you add new commits that incorporate the requested feedback, then I can review just the new commit you pushed. When rebase is used, the reviewer has to review the entire PR from scratch since every commit has changed. Rereading every single file to make sure that they are correct, every time you make a change makes the review process a lot work more for the reviewers.

Don't worry about lots of commits, you can either let me know that you want to clean up the commits once the review is complete, or what I usually do is squash the PR when I merge it so that it's a single commit.

joshuabezaleel · 2022-04-28T08:43:37Z

@carolynvs Ah, truly sorry for the inconvenience. I really thought at the beginning it was the other way around, that rebasing would make it more helpful for the reviewer, since it keeps the log straight.

When rebase is used, the reviewer has to review the entire PR from scratch since every commit has changed. Rereading every single file to make sure that they are correct, every time you make a change makes the review process a lot work more for the reviewers.

But yeah, I haven't thought of this earlier. Makes perfect sense.
Permission to confirm, so if there's conflict, I can do as follows, right?

git checkout release/v1 && git pull upstream release/v1
git checkout branch && git merge release/v1
# resolve conflict
git add . && git commit -m "Pull latest changes and resolve conflict" && git push origin branch

Now that I learned it gave the opposite effect, will definitely keep this in mind for future PRs. Thank you lots for the feedback, Carolyn!

carolynvs · 2022-04-28T19:55:18Z

Yup, doing a merge and resolving the conflicts that way are totally fine.

carolynvs

Thanks for sticking with it and getting this fix in!

carolynvs · 2022-04-28T20:06:58Z

pkg/cnab/config-adapter/stamp.go

-		data = append(append(data, m.Name...), m.Version...)
+	usedMixinsVersion := c.getUsedMixinsVersion()
+	for usedMixinName, usedMixinVersion := range usedMixinsVersion {
+		data = append(append(data, usedMixinName...), usedMixinVersion...)


I think the way we are concatenating these bytes is already pretty odd but I don't want to have you try to fix that in this PR since it's unrelated. Once your PR is in, I'll submit a PR to clean that up.

Is it because it was appended byte by byte? Should we transform everything to string first, append it one by one, then cast the finished version of it to []byte at the end?

It's just hard to read and when thinking about the digest as a whole, hard to follow what we are hashing here.

joshuabezaleel requested review from carolynvs, jeremyrickard and vdice as code owners March 1, 2022 15:44

joshuabezaleel force-pushed the 1948-bundle-mixin-version branch from dfce1ad to 19d4924 Compare March 1, 2022 15:44

joshuabezaleel marked this pull request as draft March 1, 2022 15:53

carolynvs reviewed Mar 1, 2022

View reviewed changes

carolynvs reviewed Apr 19, 2022

View reviewed changes

joshuabezaleel force-pushed the 1948-bundle-mixin-version branch 5 times, most recently from 95a01f5 to 1c1e553 Compare April 26, 2022 15:33

joshuabezaleel marked this pull request as ready for review April 26, 2022 16:02

carolynvs reviewed Apr 27, 2022

View reviewed changes

carolynvs requested changes Apr 27, 2022

View reviewed changes

joshuabezaleel force-pushed the 1948-bundle-mixin-version branch from 471754d to 2efc00a Compare April 27, 2022 16:07

joshuabezaleel added 8 commits April 27, 2022 23:15

Add mixin version when building the bundle

b5d33aa

Signed-off-by: joshuabezaleel <[email protected]>

wip

9be0ae6

Signed-off-by: joshuabezaleel <[email protected]>

wip

ba4f03e

Signed-off-by: joshuabezaleel <[email protected]>

- Rename Mixins to InstalledMixins

c177412

- Compare mixins installed with the ones defined in the manifest and then retrieved the version info - Update test to include mixin's version Signed-off-by: joshuabezaleel <[email protected]>

Fix unit test

b01f49a

Signed-off-by: joshuabezaleel <[email protected]>

Remove print statement

a860085

Signed-off-by: joshuabezaleel <[email protected]>

Fix testfiles

b3a08ac

Signed-off-by: joshuabezaleel <[email protected]>

Retrieve used mixins' version to a function to be reused

030e951

Signed-off-by: joshuabezaleel <[email protected]>

joshuabezaleel added 2 commits April 27, 2022 23:15

Add mixin version metadata to TestManifestConverter and fix test

57ccdaf

Signed-off-by: joshuabezaleel <[email protected]>

Fix manifest digest

0fc6f14

Signed-off-by: joshuabezaleel <[email protected]>

joshuabezaleel force-pushed the 1948-bundle-mixin-version branch from f31ef9b to 0fc6f14 Compare April 27, 2022 16:15

carolynvs approved these changes Apr 28, 2022

View reviewed changes

carolynvs merged commit 4a7fd67 into getporter:release/v1 Apr 28, 2022

github-actions bot mentioned this pull request Apr 28, 2022

Record the mixin version used to build the bundle #1948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mixin version when building the bundle #1952

Add mixin version when building the bundle #1952

joshuabezaleel commented Mar 1, 2022 •

edited

Loading

joshuabezaleel commented Mar 1, 2022 •

edited

Loading

carolynvs commented Mar 1, 2022

carolynvs left a comment

carolynvs commented Mar 28, 2022

joshuabezaleel commented Apr 13, 2022

carolynvs left a comment

carolynvs Apr 19, 2022

carolynvs Apr 19, 2022

joshuabezaleel Apr 26, 2022 •

edited

Loading

carolynvs Apr 27, 2022

joshuabezaleel Apr 28, 2022

carolynvs Apr 27, 2022

carolynvs left a comment

carolynvs commented Apr 27, 2022

joshuabezaleel commented Apr 28, 2022

carolynvs commented Apr 28, 2022

carolynvs left a comment

carolynvs Apr 28, 2022

joshuabezaleel Apr 29, 2022

carolynvs Apr 29, 2022

Add mixin version when building the bundle #1952

Add mixin version when building the bundle #1952

Conversation

joshuabezaleel commented Mar 1, 2022 • edited Loading

What does this change

What issue does it fix

Notes for the reviewer

Checklist

Reviewer Checklist

joshuabezaleel commented Mar 1, 2022 • edited Loading

carolynvs commented Mar 1, 2022

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs commented Mar 28, 2022

joshuabezaleel commented Apr 13, 2022

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuabezaleel Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs commented Apr 27, 2022

joshuabezaleel commented Apr 28, 2022

carolynvs commented Apr 28, 2022

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuabezaleel commented Mar 1, 2022 •

edited

Loading

joshuabezaleel commented Mar 1, 2022 •

edited

Loading

joshuabezaleel Apr 26, 2022 •

edited

Loading