docker:Image - Add flag to enable build on preview #855

guineveresaenger · 2023-11-21T20:39:46Z

This pull request's intent is to allow a user to build their image during pulumi preview by using the new buildOnPreview resource field.

This field is added to the Image schema as a boolean and defaults to false.

The implementation specifics are as follows, with some things for the reviewer to weigh in on.

As a prerequisite, supportPreview is enabled and all implemented RPD methods should handle Unknowns. This should also help address Message sooner about building for the host architecture #847 and Unable to use dynamic build property #620.
ContainsUnknowns() checks are added to the marshaler and some of the Check() logic. I wasn't sure if ContainsUnknowns() or IsComputed()should be used here; the former contains a check for the latter.
Unit tests verify the new marshaling behavior.
When a Dockerfile is Unknown, we do not verify its location during Preview Check(), instead we apply other defaults and carry on. We will calculate the build hash on the update call once Unknowns are computed.
When in preview mode, and buildOnPreview is false, we return all inputs as-is in Update() and Create().
When an attempt is made to build on preview, but there are Unknowns in the inputs or news, we send an error instructing the user to set buildOnPreview to false.
An integration test is added that verifies an image builds on preview if the buildOnPreview flag is set to true.
An integration test is added that verifies an image fails to build on preview if there are Unknown inputs and the buildOnPreview flag is set to true

Fixes #540.

Handle unknowns in Build object
Handle unknowns in Check; skip dockerfile location finding.
Set SupportPreview to true
Add ContainsUnknowns() checks for build: target, stages, platform, Dockerfile, Context; and registry: username, password, server
Add tests for Unknowns, and tweak Unknown checks as a result of a bit of TDD
Add logic to imageBuild that allows for buildOnPreview
Use Command.stdout to test unknowns
Add a few more Unknown checks in Check()
Add an integration test for Build On Preview
Build SDKs

github-actions · 2023-11-21T20:41:45Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

blampe · 2023-11-21T22:05:24Z

examples/examples_yaml_test.go

+		SkipUpdate:               true, //only run Preview
+		SkipExportImport:         true,
+		Verbose:                  true, //we need this to verify the build output logs
+		AllowEmptyPreviewChanges: true,


Wouldn't we expect to build an image in the preview?

So I think this flag is poorly named. All it does is it tells the programtest to not pass the --expect-no-changes flag to pulumi update, as the test fails spuriously otherwise. The real check is a few lines below , in whether the preview output contains messages that point to a built image. This test only runs preview, not update.

examples/test-unknowns/yaml/Dockerfile

provider/hybrid.go

examples/test-unknowns/ts/index.ts

blampe · 2023-11-21T22:44:42Z

provider/image.go

+	if !buildObject["context"].ContainsUnknowns() {
+		if buildObject["context"].IsNull() {
+			// set default
+			build.Context = "."
+		} else {
+			build.Context = buildObject["context"].StringValue()
+		}
 	}

 	// Dockerfile
-	if buildObject["dockerfile"].IsNull() {
-		// set default
-		build.Dockerfile = path.Join(build.Context, defaultDockerfile)
-	} else {
-		build.Dockerfile = buildObject["dockerfile"].StringValue()
+	if !buildObject["dockerfile"].ContainsUnknowns() {
+		if buildObject["dockerfile"].IsNull() {
+			// set default
+			build.Dockerfile = path.Join(build.Context, defaultDockerfile)
+		} else {
+			build.Dockerfile = buildObject["dockerfile"].StringValue()
+		}
 	}
+


So if I understand correctly, we'll only run a preview as long as all of our inputs have resolved. That seems reasonable. I think that matches how the bridge provider previously did these previews?

provider/provider.go

iwahbe · 2023-11-23T01:08:13Z

provider/image.go

@@ -629,7 +635,7 @@ func marshalCachedImages(b resource.PropertyValue) ([]string, error) {
 	}
 	c := b.ObjectValue()["cacheFrom"]

-	if c.IsNull() {
+	if c.IsNull() || c.IsComputed() {


Unfortunately, resource.PropertyValue.IsComputed() is a footgun.¹ Basically, resource.PropertyValue has two different ways to represent a value as "unknown": resource.Computed{ ... } and resource.Output{ Known: false, ...}. There can also be secret computed values (which this will not catch).

Suggested change

if c.IsNull() || c.IsComputed() {

if c.IsNull() || c.ContainsUnknowns() {

I can't think of a place where it would be correct to use c.IsComputed() directly in this provider.

Footnotes

https://github.com/pulumi/pulumi/issues/14620 ↩

iwahbe · 2023-11-23T01:12:29Z

provider/image.go

 	if !r.IsNull() {
-		if !r.ObjectValue()["server"].IsNull() {
+
+		if !r.ObjectValue()["server"].IsNull() && !r.ObjectValue()["server"].ContainsUnknowns() {


If r isn't an object value or null, this function will panic. I'd be very nervous here vis a vis if r is unknown or secret.

I would recommend changing the type signature of this function to operate on a resource.PropertyMap. I'm commenting on something not in this PR, so feel free to leave that for later.

iwahbe · 2023-11-23T01:13:38Z

provider/image.go

+			if !v.ContainsUnknowns() {
+				vStr := v.StringValue()
+				args[key] = &vStr
+			}


Nit: keeping things left indented

Suggested change

if !v.ContainsUnknowns() {

vStr := v.StringValue()

args[key] = &vStr

}

if v.ContainsUnknowns() {

continue

}

vStr := v.StringValue()

args[key] = &vStr

iwahbe · 2023-11-23T01:15:18Z

provider/image_test.go

+					resource.NewComputedProperty(
+						resource.Computed{Element: resource.NewStringProperty("looking-for-my-image")},
+					),


You can do this, but you don't need to.

Suggested change

resource.NewComputedProperty(

resource.Computed{Element: resource.NewStringProperty("looking-for-my-image")},

),

resource.MakeComputed(resource.NewStringProperty("looking-for-my-image")),

This applies in many test cases.

iwahbe · 2023-11-23T01:18:59Z

provider/provider.go

+		knownDockerfile = true
+	} else {
+		// We do not want to set these fields if their values are Unknown.
+		if !inputs["build"].ObjectValue()["dockerfile"].ContainsUnknowns() {


It looks like if inputs["build"] is computed, then this will panic. Am I missing a check somewhere?

I did think about this and was having a difficult time coming up with a practical example of a situation where a program would pass in the entire build object as Unknown.
It's not impossible, and definitely worth a safety check, but my thinking was, in most situations unknowns would be passed to the concrete fields, not the overall build object (and I haven't seen any bugs filed that showed this kind of pattern).

It's weird but definitely possible! I've done things like this:

build: thing.arn.apply((arn) => { return { dockerfile: "Dockerfile", args: { THE_ARN: arn } } })

Sure you could have done that apply deeper in the object... but anything is possible :)

I added a small test to sanity check everything still works when build is computed and it caught a few panics... whew!

iwahbe · 2023-11-23T01:22:15Z

provider/provider.go

@@ -325,6 +334,18 @@ func (p *dockerNativeProvider) Create(ctx context.Context, req *rpc.CreateReques
 		return nil, errors.Wrapf(err, "malformed resource inputs")
 	}

+	// if we're in preview mode and buildOnPreview is set to false, we return the inputs
+	if req.GetPreview() && !inputs["buildOnPreview"].BoolValue() {


What if inputs["buildOnPreview"] is unknown?

This code will panic.

What should the provider do?

Again, thinking about this from a user perspective, I was having a difficult time coming up with a situation where anyone would make buildOnPreview unknown, but you're right that the world is a large and diverse place so we should add a check. My feeling is to warn and continue.

iwahbe · 2023-11-23T01:24:51Z

provider/provider.go

+	if req.GetPreview() && !inputs["buildOnPreview"].BoolValue() {
+		return &rpc.CreateResponse{
+			Properties: req.GetProperties(),
+		}, nil
+	}
+	// buildOnPreview needs all inputs to be resolved. Return error if trying to build on preview and there are Unknowns.
+	if req.GetPreview() && inputs["buildOnPreview"].BoolValue() && inputs.ContainsUnknowns() {
+		return nil, errors.New("cannot build on preview with unresolved inputs. " +
+			"Set buildOnPreview to False, or ensure all inputs are resolved at preview.")
+	}


Code structure: Can we fold the req.GetPreview() call one level higher:

Suggested change

if req.GetPreview() && !inputs["buildOnPreview"].BoolValue() {

return &rpc.CreateResponse{

Properties: req.GetProperties(),

}, nil

}

// buildOnPreview needs all inputs to be resolved. Return error if trying to build on preview and there are Unknowns.

if req.GetPreview() && inputs["buildOnPreview"].BoolValue() && inputs.ContainsUnknowns() {

return nil, errors.New("cannot build on preview with unresolved inputs. " +

"Set buildOnPreview to False, or ensure all inputs are resolved at preview.")

}

if req.GetPreview() {

if !inputs["buildOnPreview"].BoolValue() {

return &rpc.CreateResponse{

Properties: req.GetProperties(),

}, nil

}

// buildOnPreview needs all inputs to be resolved. Return error if trying to build on preview and there are Unknowns.

if inputs.ContainsUnknowns() {

return nil, errors.New("cannot build on preview with unresolved inputs. " +

"Set buildOnPreview to False, or ensure all inputs are resolved at preview.")

}

}

iwahbe · 2023-11-23T01:25:41Z

provider/provider.go

+	}
+	// buildOnPreview needs all inputs to be resolved. Return error if trying to build on preview and there are Unknowns.
+	if req.GetPreview() && inputs["buildOnPreview"].BoolValue() && inputs.ContainsUnknowns() {
+		return nil, errors.New("cannot build on preview with unresolved inputs. " +


It feels like this should be a warning instead of an error. If we have agreed that this should error in the past, please ignore this comment.

iwahbe · 2023-11-27T18:59:49Z

provider/provider.go

+		// buildOnPreview needs all inputs to be resolved. Warn and continue without building the image
+		// TODO: there is room for some future granularity here - we should be able to build a local image without
+		// (TODO cont) knowing inputs for a registry, for example.
+		if inputs["buildOnPreview"].BoolValue() && inputs.ContainsUnknowns() {


We already know here that inputs["buildOnPreview"].BoolValue() is true, since if it wasn't we would have exited the function on L352.

Suggested change

if inputs["buildOnPreview"].BoolValue() && inputs.ContainsUnknowns() {

if inputs.ContainsUnknowns() {

provider/provider.go

iwahbe

Barring the comments I have already made, this looks good vis a vis resource.PropertyValue handling. I'll leave it to @blampe to check that the PR does what it is supposed to.

provider/hybrid.go

iwahbe · 2023-11-27T23:29:23Z

provider/image.go

@@ -639,38 +645,38 @@ func marshalCachedImages(b resource.PropertyValue) ([]string, error) {
 	if !ok {
 		return cacheImages, fmt.Errorf("cacheFrom requires an `images` field")
 	}
-	if images.IsNull() {
+	if images.IsNull() || images.IsComputed() {


Suggested change

if images.IsNull() || images.IsComputed() {

if images.IsNull() || images.ContainsUnknowns() {

iwahbe · 2023-11-27T23:32:02Z

provider/provider.go

+
+func marshalBuildOnPreview(inputs resource.PropertyMap) bool {
+	//set default if not set
+	if inputs["buildOnPreview"].IsNull() || inputs["buildOnPreview"].IsComputed() {


Suggested change

if inputs["buildOnPreview"].IsNull() || inputs["buildOnPreview"].IsComputed() {

if inputs["buildOnPreview"].IsNull() || inputs["buildOnPreview"].ContainsUnknowns() {

blampe

Looking good, only some minor nits and one high-level question that's not blocking.

blampe · 2023-11-28T20:35:59Z

provider/cmd/pulumi-resource-docker/schema.json

+                "buildOnPreview": {
+                    "type": "boolean",
+                    "description": "A flag to build an image on preview",
+                    "default": false
+                },


I realize it's late in the game to be asking this, but I'm curious why this is a resource property and not some configuration on the provider.

With 3.x, it would build all images on preview. So, strictly speaking, for anyone that wants that behavior back it seems like it would be OK to set at the provider level.

I'm not saying that because I think we need to do it, only to make sure we've considered it. (If anything, doing it this way might make the buildx.Image resource slightly easier because we could defer or punt on this behavior altogether.)

What if someone wants to build some images on preview, but not others? I could easily imagine only wanting to check this conditionally. I opted for flexibility here, rather than "all or nothing", but I admit I did not consider inheriting this for other inflections of Image.

blampe · 2023-11-28T20:37:50Z

provider/image.go

+		if !images.ContainsUnknowns() {
+			return cacheImages, fmt.Errorf("the `images` field must be a list of strings")
+		} else {
+			return cacheImages, nil
+		}
+


Suggested change

if !images.ContainsUnknowns() {

return cacheImages, fmt.Errorf("the `images` field must be a list of strings")

} else {

return cacheImages, nil

}

if !images.ContainsUnknowns() {

return cacheImages, fmt.Errorf("the `images` field must be a list of strings")

}

return cacheImages, nil

blampe · 2023-11-28T20:42:14Z

provider/provider.go

+	} else {
+		// avoid panic if inputs["build"] is not an Object - we only want to set these fields if their values are Known.
+		if inputs["build"].IsObject() {
+			if !inputs["build"].ObjectValue()["dockerfile"].ContainsUnknowns() {
+				inputs["build"].ObjectValue()["dockerfile"] = resource.NewStringProperty(build.Dockerfile)
+				knownDockerfile = true
+			}
+			if !inputs["build"].ObjectValue()["context"].ContainsUnknowns() {
+				inputs["build"].ObjectValue()["context"] = resource.NewStringProperty(build.Context)
+			}
+		}
+	}


Suggested change

} else {

// avoid panic if inputs["build"] is not an Object - we only want to set these fields if their values are Known.

if inputs["build"].IsObject() {

if !inputs["build"].ObjectValue()["dockerfile"].ContainsUnknowns() {

inputs["build"].ObjectValue()["dockerfile"] = resource.NewStringProperty(build.Dockerfile)

knownDockerfile = true

}

if !inputs["build"].ObjectValue()["context"].ContainsUnknowns() {

inputs["build"].ObjectValue()["context"] = resource.NewStringProperty(build.Context)

}

}

}

} else if inputs["build"].IsObject() {

// avoid panic if inputs["build"] is not an Object - we only want to set these fields if their values are Known.

if !inputs["build"].ObjectValue()["dockerfile"].ContainsUnknowns() {

inputs["build"].ObjectValue()["dockerfile"] = resource.NewStringProperty(build.Dockerfile)

knownDockerfile = true

}

if !inputs["build"].ObjectValue()["context"].ContainsUnknowns() {

inputs["build"].ObjectValue()["context"] = resource.NewStringProperty(build.Context)

}

}

blampe · 2023-11-28T20:45:49Z

provider/provider.go

+
+		if _, statErr := os.Stat(build.Dockerfile); statErr != nil {
+			if filepath.IsAbs(build.Dockerfile) {
+				return nil, fmt.Errorf("could not open dockerfile at absolute path %s: %v", build.Dockerfile, statErr)


Nit: instead of "could not open..." a more helpful error might say something like "expected a relative path, got %q instead".

If build object contains unknowns, skip Dockerfile hashing and platform logic in Check(). This is a blunt approach, as it doesn't allow us to granularly apply verificaton logic the first time Chieck() is called.

…ckerfile, Context; and registry: username, password, server

… of TDD

It turns out the random provider does not in fact return computed values, so this test never tested what it said to test. Using Command.stdout fixes this.

…and Create when run in preview mode. Disambiguate test name.

…ne is trying to build on preview with unresolved inputs.

…ainsUnknowns() for cacheFrom and cacheFrom.Images

… preview build

guineveresaenger · 2023-12-05T22:45:00Z

provider/provider.go

+	if inputs["buildOnPreview"].ContainsUnknowns() {
+		msg = "buildOnPreview is unresolved; cannot build on preview. Continuing without preview image build. " +
+			"To avoid this warning, set buildOnPreview explicitly, and ensure all inputs are resolved at preview."
+		returnWithoutBuild = true


could we return early here? then we don't need to do the double verification on L#755.
We could return msg instead of an error, and put the logging logic and error handling in the main function.

Great idea, fixed!

guineveresaenger · 2023-12-05T22:47:35Z

provider/provider_test.go

@@ -458,3 +458,93 @@ func TestCheck(t *testing.T) {
 		})
 	}
 }
+
+func TestCanPreview(t *testing.T) {


It's a little more work up front to crank out some unit tests but worth it for the peace of mind!

blampe

Looks great! Let's cut a prerelease and give this a spin internally! I think there's already a 4.6.0-alpha.0 if it matters.

blampe · 2023-12-05T21:46:13Z

examples/aws-container-registry/ts/package.json

    "dependencies": {
+        "@pulumi/aws": "^6.10.0",
        "@pulumi/pulumi": "^3.0.0",
-        "@pulumi/aws": "latest",
-        "@pulumi/random": "latest"
+        "@pulumi/random": "^4.14.0"


Unrelated to your PR but shouldn't these include @pulumi/docker? I was a little confused when I tried running one locally and it didn't work.

blampe · 2023-12-05T21:49:35Z

provider/image_test.go

 		})
 		actual, err := marshalCachedImages(buildInput)
 		assert.NoError(t, err)
 		assert.Equal(t, expected, actual)
 	})
 }

+// TODO: do we want to allow Builder to be Unknown? there's very little use case here


I think we can skip this one, very few people are setting this in the wild so I'm not too concerned about it working in exotic ways.

blampe · 2023-12-05T21:50:36Z

provider/image_test.go

@@ -366,6 +477,7 @@ func TestMarshalBuilder(t *testing.T) {
 	})
 }

+// TODO: do we want to allow SkipPush to be Unknown?


This one is more plausible and fortunately you've already handled it!

blampe · 2023-12-05T21:50:55Z

provider/provider.go

@@ -134,58 +133,86 @@ func (p *dockerNativeProvider) log(ctx context.Context, sev diag.Severity, urn r
 // the provider inputs are using for detecting and rendering diffs.
 func (p *dockerNativeProvider) Check(ctx context.Context, req *rpc.CheckRequest) (*rpc.CheckResponse, error) {
 	urn := resource.URN(req.GetUrn())
-	label := fmt.Sprintf("%s.Create(%s)", p.name, urn)
+	label := fmt.Sprintf("%s.Check(%s)", p.name, urn)


blampe · 2023-12-05T23:11:54Z

provider/provider.go

+		knownDockerfile = true
+	} else {
+		// We do not want to set these fields if their values are Unknown.
+		if !inputs["build"].ObjectValue()["dockerfile"].ContainsUnknowns() {


It's weird but definitely possible! I've done things like this:

build: thing.arn.apply((arn) => { return { dockerfile: "Dockerfile", args: { THE_ARN: arn } } })

Sure you could have done that apply deeper in the object... but anything is possible :)

I added a small test to sanity check everything still works when build is computed and it caught a few panics... whew!

blampe · 2023-12-05T23:12:17Z

provider/provider.go

+	if inputs["buildOnPreview"].ContainsUnknowns() {
+		msg = "buildOnPreview is unresolved; cannot build on preview. Continuing without preview image build. " +
+			"To avoid this warning, set buildOnPreview explicitly, and ensure all inputs are resolved at preview."
+		returnWithoutBuild = true


Great idea, fixed!

blampe · 2023-12-05T23:13:54Z

provider/provider_test.go

@@ -458,3 +458,93 @@ func TestCheck(t *testing.T) {
 		})
 	}
 }
+
+func TestCanPreview(t *testing.T) {


It's a little more work up front to crank out some unit tests but worth it for the peace of mind!

guineveresaenger marked this pull request as draft November 21, 2023 20:42

blampe reviewed Nov 21, 2023

View reviewed changes

guineveresaenger marked this pull request as ready for review November 22, 2023 20:27

guineveresaenger requested review from blampe and a team November 22, 2023 20:27

guineveresaenger mentioned this pull request Nov 22, 2023

Handle unknowns in Build object #747

Closed

iwahbe reviewed Nov 23, 2023

View reviewed changes

guineveresaenger requested a review from iwahbe November 27, 2023 18:54

iwahbe reviewed Nov 27, 2023

View reviewed changes

provider/provider.go Show resolved Hide resolved

iwahbe reviewed Nov 27, 2023

View reviewed changes

blampe reviewed Nov 28, 2023

View reviewed changes

guineveresaenger force-pushed the guin/build-on-preview branch from 76b2863 to 545c35b Compare December 4, 2023 22:24

guineveresaenger added 15 commits December 4, 2023 14:25

Handle unknowns in Build object

6e18d4a

If build object contains unknowns, skip Dockerfile hashing and platform logic in Check(). This is a blunt approach, as it doesn't allow us to granularly apply verificaton logic the first time Chieck() is called.

Handle unknowns in Check; skip dockerfile location finding.

171d637

Set SupportPreview to true

bfedb3c

Add ContainsUnknowns() checks for build: target, stages, platform, Do…

0cd8da6

…ckerfile, Context; and registry: username, password, server

Add tests for Unknowns, and tweak Unknown checks as a result of a bit…

3190f9e

… of TDD

Add logic to imageBuild that allows for buildOnPreview

e22f19e

Use Command.stdout to test unknowns

997c88d

It turns out the random provider does not in fact return computed values, so this test never tested what it said to test. Using Command.stdout fixes this.

Add a few more Unknown checks in Check()

e827fd6

Add an integration test for Build On Preview

d80576e

Build SDKs

55d1e37

cleanup of comments

33a1619

fix linter and output values for Unknowns tests

f12054a

Do not put runBuild? logic into dockerBuild. Return inputs to Update …

c616e3d

…and Create when run in preview mode. Disambiguate test name.

Fix knownDockerfile oversight. Add safety check which errors if someo…

7202983

…ne is trying to build on preview with unresolved inputs.

Add test that shows the provider rejects unknowns in a preview build

45b2750

guineveresaenger added 8 commits December 4, 2023 14:33

address code review

0f0376c

fix linter

f23c4a9

add parameter to make code more readable

daeafeb

Remove old comment and use ContainsUnknowns()

6af66a6

Use IsObject() and IsArray() checks rather than the too-powerful Cont…

2e6a0d4

…ainsUnknowns() for cacheFrom and cacheFrom.Images

fix unknowns test to no longer expect a failure. Pacify linter.

82c06ce

Add isPreview param to and ensure minimum required inputs for a local…

9a8b1e2

… preview build

clean up from merge

96d5cf1

guineveresaenger force-pushed the guin/build-on-preview branch from 545c35b to 96d5cf1 Compare December 4, 2023 22:46

blampe added 6 commits December 4, 2023 15:24

fix swarm test

7ee8558

Don't build if args contains unknowns; check imageName for stringness

0b5b170

fix error message

2795a1f

add a test for imageName validation when unknown

a53974a

consolidate and test canPreview logic

a7e7734

lint

d196772

guineveresaenger commented Dec 5, 2023

View reviewed changes

blampe added 3 commits December 5, 2023 15:02

handle unknown build

edf3ce4

return early from canPreview

413e3ad

comment

1826199

blampe approved these changes Dec 5, 2023

View reviewed changes

guineveresaenger merged commit 11599a3 into master Dec 5, 2023
17 checks passed

guineveresaenger deleted the guin/build-on-preview branch December 5, 2023 23:46

	if c.IsNull() \|\| c.IsComputed() {
	if c.IsNull() \|\| c.ContainsUnknowns() {

	if inputs["buildOnPreview"].BoolValue() && inputs.ContainsUnknowns() {
	if inputs.ContainsUnknowns() {

	if images.IsNull() \|\| images.IsComputed() {
	if images.IsNull() \|\| images.ContainsUnknowns() {

	if inputs["buildOnPreview"].IsNull() \|\| inputs["buildOnPreview"].IsComputed() {
	if inputs["buildOnPreview"].IsNull() \|\| inputs["buildOnPreview"].ContainsUnknowns() {

docker:Image - Add flag to enable build on preview #855

docker:Image - Add flag to enable build on preview #855

Conversation

guineveresaenger commented Nov 21, 2023 • edited Loading

github-actions bot commented Nov 21, 2023

Does the PR have any schema changes?

Does the PR have any schema changes?

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

Footnotes

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwahbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blampe 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blampe 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

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

guineveresaenger commented Nov 21, 2023 •

edited

Loading