[devbox] Make stack-examples using Postgres actually work #874

savil · 2023-04-07T00:31:11Z

Summary

Added set -e in devbox.json of lapp-stack and lepp-stack examples. This exposed failure scenarios in devbox run run_test:

Postgres's unix socket path was too long. Fix: see below.
curl was missing from packages. Fix: add curl to packages.
the postgres ports in the examples needed to be unique so they can run in parallel. Fix: Set PGPORT in env of devbox.json, and updated other examples code that referenced the db-port.
in lepp-stack, the nginx port of 80 was not working in Github Actions. Fix: Changed to 8089.

Fix for "unix socket path too long" error

Unix socket paths need to be less than approx. 100 chars long. This is a legacy limitation.

Postgres's socket path is determined by the PGHOST location.
- in plugins/postgresql.json, we set PGHOST to Virtenv template data.
- We make a symlink from XDG_STATE_HOME/devbox/v-<projectDir hash> to <project dir>/.devbox/virtenv, and set the template's Virtenv data to be this symlink's path: XDG_STATE_HOME/devbox/v-<projectDir hash>/<plugin>

Test Plan

to run the lapp-stack and lepp-stack:

time DEVBOX_EXAMPLE_TESTS=1 DEVBOX_DEBUG=0 go test -count 1 -v -run TestExamples/stacks_l ./testscripts/...

I also want to add a unit-test for virtenvSymlinkPath

debugging pro-tip:
When there is a failure, there will often be active postgres processes. Trying to run run_test again may lead to issues, so pkill -f postgres them first. This is because failures in run_test prevent devbox services stop from cleaning these processes up.

savil · 2023-04-07T00:31:23Z

Current dependencies on/for this PR:

main
- PR set HOME for testscripts #875
  - PR [devbox] Make stack-examples using Postgres actually work #874 👈
    - PR [examples] enable stacks-django and rust examples to pass #880
      - PR [example tests] generate "set -e" to exit scripts upon failure #869

This comment was auto-generated by Graphite.

mikeland73

This feels overly complex and outside of the realm of what devbox should care about. I'm all for making sure our tests can run, but this:

Introduces significant complexity
Generates different symlinks based on the depth of the project in the filesystem. (this in particular fells bad). It creates some branching that I'm afraid could cause issues in the future.
Can generate different sym links depending on package name.

Another alternative:

Have CreateFilesAndShowReadme create a symlink to [project]/.devbox/virtenv. Keep this simple and deterministic (so don't change methods depending on length).
Replace existing virtenv variable in the templates with [symlink]/[pgk]
If [symlink]/[pgk] is longer than 80 chars, show a warning. This should be incredibly rare. I'm not even sure we should do this, since this is an issue every developer has to deal with and is independent of devbox.

mikeland73 · 2023-04-07T22:39:51Z

internal/plugin/plugin.go

+func ensureVirtenvPath(projectDir string) (string, error) {
+	path := filepath.Join(projectDir, VirtenvPath)
+	if err := createDir(path); err != nil {
+		return "", err
+	}
+	return path, nil
+}
+
+func ensureVirtenvPathForPackage(projectDir, pkg string) (string, error) {
+	virtenvPath, err := ensureVirtenvPath(projectDir)
+	if err != nil {
+		return "", err
+	}
+	path := filepath.Join(virtenvPath, pkg)
+	if err := createDir(path); err != nil {
+		return "", err
+	}
+	return path, nil
+}


Does CreateFilesAndShowReadme not already do this?

https://github.com/jetpack-io/devbox/blob/main/internal/plugin/plugin.go#L62-L66

I ran into some failure cases. It seems this function is called from other callpaths as well.

its possible, this was a false impression I got. I can remove these two ensure functions.

mikeland73 · 2023-04-07T22:42:35Z

internal/plugin/plugin.go

+		"Virtenv":              virtenvPathForPackage,
+		"VirtenvRuntimePath":   filepath.Join(virtenvXdgRuntimePath, pkg),


I would just use Virtenv and have the symlink in there. So Virtenv becomes:

~/.local/run/[project-hash]/[pkg]

instead of having both (Virtenv and VirtenvRuntimePath)

ah, okay, I can do that.

mikeland73 · 2023-04-07T22:43:59Z

internal/plugin/files.go

+	xdgRuntimePath, err := setupXdgRuntimePath(projectDir)
+	if err != nil {
+		return nil, err
+	}
+


I'm not sure this is the best place to do this. We will run this on every config we load. (so we're just running this multiple times unnecessarily).

CreateFilesAndShowReadme which is the main API for plugins could ensure the symlink is created

Ack, on this being sub-optimal. I have a TODO below in line 65 to improve this.

getConfigIfAny is used in quite a few places:

❯ git grep -n getConfigIfAny internal/plugin/files.go:17:func getConfigIfAny(pkg, projectDir string) (*config, error) { internal/plugin/hooks.go:6: c, err := getConfigIfAny(pkg, projectDir) internal/plugin/info.go:12: cfg, err := getConfigIfAny(pkg, projectDir) internal/plugin/plugin.go:54: cfg, err := getConfigIfAny(pkg, projectDir) internal/plugin/plugin.go:136: cfg, err := getConfigIfAny(pkg, projectDir) internal/plugin/services.go:10: conf, err := getConfigIfAny(pkg, projectDir)

So, not just CreateFilesAndShowReadme.

moved TODO to this callsite.

getConfigIfAny is a bit of a general load function for a single plugin, while the symlink is per-project. So IMO it makes sense to create in the same place we ensure virtenv directory is created (since that is what the symlink is supposed to point to)

I can create it there, but I still need to pass it in. GetConfigIfAny is called from many different code paths. This is why I have added a TODO to improve the code more holistically in this respect.

CreateFilesAndShowReadme gets called on every ensurePackagesAreInstalled so it's where you want this.

It's not the best API, but I'm confident you will get the symlink you want.

CreateFilesAndShowReadme creates the virtenv directory. It creates any support files needed for plugins (e.g configs, etc). Services cannot run without these files which is why I think it makes the most sense for us to create the symlink there.

The symlink path is computed deterministically using projectDir which means GetConfigIfAny can just get the Symlink path without having to try and re-create it.

mikeland73 · 2023-04-07T22:45:31Z

internal/plugin/files.go

+		if len([]byte(xdgRuntimeDir)) < maxDirLen {
+			if err := os.MkdirAll(xdgRuntimeDir, 0700); err != nil {
+				return "", errors.WithStack(err)
+			}
+			return xdgRuntimeDir, nil
+		}
+
+		// TODO hook up to correct io.writer
+		//ux.Fwarning("xdgRuntimeDir is too long for unix-socket paths, "+
+		//	"falling back to /tmp/user-<uid>. xdgRuntime: %s\n", xdgRuntimeDir)
+
+		// Fallback to /tmp/user-<uid>/devbox/ dir
+		// We do not use os.TempDir() since it can be pretty long in MacOS.
+		// /tmp is posix-compliant and expected to exist, and is short.
+		dir := filepath.Join(fmt.Sprintf("/tmp/user-%d", os.Getuid()), "devbox")
+		if err := os.MkdirAll(dir, 0700); err != nil {
+			return "", errors.WithStack(err)
+		}
+


I'm not sure this complexity is really worth it. If the user nests their project so deep that sockets are invalid, it's not really up to devbox to fix it.

I disagree here. Devbox is in charge of deciding the path to where the socket lives, so it is up to devbox to fix it.

I'm with you on providing a symlink, but not on splitting the implementation. It's code that almost certainly will never get exercised.

if a users ~[HOME]/.local/run/v-12345/postgres/socket.12435 is more than 100 characters it means their username is 50 characters. The maximum on linux os 32.

mikeland73 · 2023-04-07T22:54:53Z

internal/plugin/files.go

+	deriveLinkName := func(projectDir string) (string, error) {
+		h := fnv.New32a()
+		_, err := h.Write([]byte(projectDir))
+		if err != nil {
+			return "", errors.WithStack(err)
+		}
+		hashed := strconv.FormatUint(uint64(h.Sum32()), 10)
+		if len(hashed) > virtenvPathHashLength { // the length is expected to be 10 but being defensive.
+			hashed = hashed[:virtenvPathHashLength]
+		}
+		linkName := fmt.Sprintf("virtenv-%s", hashed)
+		return linkName, nil
+	}


Why not just use md5? Also use base64 to make it smaller, 5 chars is more than enough entropy.

Finally you can name it v-[hash]

I wanted a non-crypto hash so its faster. Not sure md5 provides any meaningful benefit here.

other points are 👍🏾 on 5 chars and v-hash

savil

I disagree on your assessment that its adding complexity for a scenario that we need not support. This scenario happens in both real-world situations and tests (which are also real-world situations: our users can be expected to write CICD tests for their own projects).

So I am going to update but preserve the fallback so that this scenario can be robust to failure.

Happy to discuss more.

savil · 2023-04-07T23:31:54Z

internal/plugin/files.go

+	xdgRuntimePath, err := setupXdgRuntimePath(projectDir)
+	if err != nil {
+		return nil, err
+	}
+


Ack, on this being sub-optimal. I have a TODO below in line 65 to improve this.

getConfigIfAny is used in quite a few places:

❯ git grep -n getConfigIfAny internal/plugin/files.go:17:func getConfigIfAny(pkg, projectDir string) (*config, error) { internal/plugin/hooks.go:6: c, err := getConfigIfAny(pkg, projectDir) internal/plugin/info.go:12: cfg, err := getConfigIfAny(pkg, projectDir) internal/plugin/plugin.go:54: cfg, err := getConfigIfAny(pkg, projectDir) internal/plugin/plugin.go:136: cfg, err := getConfigIfAny(pkg, projectDir) internal/plugin/services.go:10: conf, err := getConfigIfAny(pkg, projectDir)

So, not just CreateFilesAndShowReadme.

savil · 2023-04-07T23:33:01Z

internal/plugin/files.go

+	deriveLinkName := func(projectDir string) (string, error) {
+		h := fnv.New32a()
+		_, err := h.Write([]byte(projectDir))
+		if err != nil {
+			return "", errors.WithStack(err)
+		}
+		hashed := strconv.FormatUint(uint64(h.Sum32()), 10)
+		if len(hashed) > virtenvPathHashLength { // the length is expected to be 10 but being defensive.
+			hashed = hashed[:virtenvPathHashLength]
+		}
+		linkName := fmt.Sprintf("virtenv-%s", hashed)
+		return linkName, nil
+	}


I wanted a non-crypto hash so its faster. Not sure md5 provides any meaningful benefit here.

other points are 👍🏾 on 5 chars and v-hash

savil · 2023-04-07T23:33:38Z

internal/plugin/files.go

+		if len([]byte(xdgRuntimeDir)) < maxDirLen {
+			if err := os.MkdirAll(xdgRuntimeDir, 0700); err != nil {
+				return "", errors.WithStack(err)
+			}
+			return xdgRuntimeDir, nil
+		}
+
+		// TODO hook up to correct io.writer
+		//ux.Fwarning("xdgRuntimeDir is too long for unix-socket paths, "+
+		//	"falling back to /tmp/user-<uid>. xdgRuntime: %s\n", xdgRuntimeDir)
+
+		// Fallback to /tmp/user-<uid>/devbox/ dir
+		// We do not use os.TempDir() since it can be pretty long in MacOS.
+		// /tmp is posix-compliant and expected to exist, and is short.
+		dir := filepath.Join(fmt.Sprintf("/tmp/user-%d", os.Getuid()), "devbox")
+		if err := os.MkdirAll(dir, 0700); err != nil {
+			return "", errors.WithStack(err)
+		}
+


I disagree here. Devbox is in charge of deciding the path to where the socket lives, so it is up to devbox to fix it.

savil · 2023-04-07T23:38:03Z

internal/plugin/plugin.go

+		"Virtenv":              virtenvPathForPackage,
+		"VirtenvRuntimePath":   filepath.Join(virtenvXdgRuntimePath, pkg),


ah, okay, I can do that.

savil · 2023-04-07T23:38:39Z

internal/plugin/plugin.go

+func ensureVirtenvPath(projectDir string) (string, error) {
+	path := filepath.Join(projectDir, VirtenvPath)
+	if err := createDir(path); err != nil {
+		return "", err
+	}
+	return path, nil
+}
+
+func ensureVirtenvPathForPackage(projectDir, pkg string) (string, error) {
+	virtenvPath, err := ensureVirtenvPath(projectDir)
+	if err != nil {
+		return "", err
+	}
+	path := filepath.Join(virtenvPath, pkg)
+	if err := createDir(path); err != nil {
+		return "", err
+	}
+	return path, nil
+}


I ran into some failure cases. It seems this function is called from other callpaths as well.

savil · 2023-04-07T23:50:56Z

internal/plugin/files.go

+// The symlink enables the path to be short so that unix sockets can be placed
+// in this directory. Unix sockets have legacy ~100 char limits for their paths.
+//
+// TODO savil. Calculate this once for each plugin, and reuse.


this should be: once for each project

savil · 2023-04-08T01:54:27Z

Can generate different sym links depending on package name.

this is also not accurate. The symlinks are one per project. setupXdgRuntimePath(projectDir string) doesn't have any parameters specifying the package-name.

mikeland73 · 2023-04-08T01:57:13Z

I disagree on your assessment that its adding complexity for a scenario that we need not support. This scenario happens in both real-world situations and tests (which are also real-world situations: our users can be expected to write CICD tests for their own projects).

I think this is a nice thing to do for users, but like everything there are tradeoffs. I do think this going beyond what devbox needs to do (user could create symlink themselves), but given that we want to provide a good user experience I think creating the symlink is the way to go. That said, splitting the implementation feels like code that is (almost?) impossible to exercise at the expense of leaving code in that we might never feel confident removing (it's always harder to remove stuff like this).

mikeland73 · 2023-04-08T02:08:40Z

internal/plugin/files.go

 	"go.jetpack.io/devbox/plugins"
 )

-func getConfigIfAny(pkg, projectDir string) (*config, error) {
+func getConfigIfAny(pkg, projectDir, xdgRuntimePath string) (*config, error) {


instead of passing xdgRuntimePath here, what do you think of the following API:

plugin.VirtenvSymlinkPath(projectDir string) string plugin.CreateVirtenvSymlink(projectDir string) string

CreateVirtenvSymlink calls VirtenvSymlinkPath to get path it needs to create.

CreateFilesAndShowReadme calls CreateVirtenvSymlink to create the symlink if needed

getConfigIfAny calls VirtenvSymlinkPath to populate virtenv variable.

I like it +1

mikeland73 · 2023-04-08T02:09:06Z

internal/plugin/files.go

+//
+// The symlink enables the path to be short so that unix sockets can be placed
+// in this directory. Unix sockets have legacy ~100 char limits for their paths.
+func setupXdgRuntimePath(projectDir string) (string, error) {


nit, rename to CreateVirtenvSymlink

mikeland73 · 2023-04-08T02:09:20Z

internal/plugin/files.go

+
+// virtenvRuntimeLinkPath returns a path for the given project's virtenv's Runtime resources to live in.
+// It strives to be XDG compliant, but will diverge if needed to be short enough to be used in unix sockets.
+func virtenvRuntimeLinkPath(projectDir string) (string, error) {


nit rename to VirtenvSymlinkPath

mikeland73

Could you put CreateVirtenvSymlink and VirtenvSymlinkPath into new file? (maybe symlink.go)

savil · 2023-04-08T03:07:55Z

@mikeland86 thanks for the after-hours review. Let me think about this over the weekend and get it to a place we're all happy with.

savil · 2023-04-10T18:47:30Z

@mikeland86 I took to heart your feedback about designing this properly, since it'll be hard to make changes later.

Falling back to /tmp: lets drop it

I think this is a nice thing to do for users, but like everything there are tradeoffs. I do think this going beyond what devbox needs to do (user could create symlink themselves), but given that we want to provide a good user experience I think creating the symlink is the way to go. That said, splitting the implementation feels like code that is (almost?) impossible to exercise at the expense of leaving code in that we might never feel confident removing (it's always harder to remove stuff like this).

For completeness, I'll add: the fallback path gets exercised in the unit-test, and in the examples testscripts. In our testscripts, we set HOME to be t.TempDir() which tends to be long (in #875).

That said, I agree that for realistic user scenarios having the XDG_RUNTIME_PATH is sufficient for postgres plugins. The fallback /tmp path is unlikely to be needed.

Lets drop the /tmp fallback in the core devbox code, and add a custom XDG_RUNTIME_PATH for examples testscripts that will be short enough. This is point (3) in the proposal below.

Symlink has problems

Reading this PR again, I find linking XDG_RUNTIME_DIR to .devbox/virtenv problematic: we put various kinds of data into virtenv folders, some of which are XDG_DATA_DIR and XDG_STATE_DIR suitable. This seems not great.

The spec on XDG_RUNTIME_DIR states:

The lifetime of the directory MUST be bound to the user being logged in. It MUST be created when the user first logs in and if the user fully logs out the directory MUST be removed.....Files in the directory MUST not survive reboot or a full logout/login cycle.
https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

I don't think we need be concerned about managing the lifetime of the XDG_RUNTIME_DIR; that seems the OS's job, but likely best to not have a link pointing to all of virtenv. If some OS is clearing data from the XDG_RUNTIME_DIR, then I worry it'll delete all the virtenv directories too 😱

Suggested fix:

In the plugin.config spec, add a field for use_xdg_runtime_symlink: bool. For plugins/postgresql.json, set this to true.
For plugins with use_xdg_runtime_symlink, create a symlink {$XDG_RUNTIME_PATH}/devbox/p-<hash> --> {projectDir}/virtenv/<plugin>/run. NOTE: this is per-plugin, and no longer per-project.
For plugin templates, revert back to having VirtenvRuntime field, distinct from the old Virtenv field. Only the postgres plugin need set PGHOST: {{ .VirtenvRuntime }}. Other plugins can use it when needed.

My proposal to move forward is:

Symlink suggested fix above.
switch to plugin.createVirtenvRuntimeSymlink and plugin.virtenvRuntimeSymlinkPath APIs, as you suggest: call createVirtenvRuntimeSymlink in CreateFilesAndShowReadme. Call virtenvRuntimeSymlinkPath inside plugin.getConfigIfAny.
drop the fallback to /tmp/user-123 in the core devbox code, and set env XDG_RUNTIME_PATH=/tmp/devbox-examples-testscripts for the examples testscripts.

What do you think?

savil · 2023-04-10T19:05:33Z

Spoke to Mike. Instead of (1) in the proposal for per-plugin link from XDG_RUNTIME_DIR, we'll have: XDG_STATE_HOME/devbox/v-<hash> point to ProjectDir/.devbox/virtenv. That greatly simplifies the proposal above.

savil · 2023-04-10T22:44:25Z

@mikeland86 PTAL

gcurtis

Thanks for fixing all this -- I know it hasn't been straightforward. You mentioned you're planning on changing the hash, so LGTM.

gcurtis · 2023-04-11T22:49:59Z

examples/stacks/lapp-stack/devbox.json

@@ -14,6 +18,8 @@
                "psql devbox_lapp < setup_postgres_db.sql"
            ],
            "run_test": [
+                "set -e",
+                "rm -rf $PGDATA",


Is this dangerous? Could there ever be a bug where the user's normal $PGDATA is set and it deletes their database?

Oh I think I don't need this... I added it while debugging and forgot to remove I think.

…t path is too long" error

…sureVirtenvXYZ functions, use 5 length hash, and minor other changes

…kPath (3) override XDG_STATE_HOME for examples testscripts

mikeland73

Nice!

## Summary Remove virtualenv data xdg directory symlinks. The current state is quite hard to reason about for the following reason: ### Diverging source of truth We have `.devbox/virtualenv` which supposedly should be the source of truth for plugins. However, upon inspection, it looks like the paths and variables set for `{{ .Virtualenv }}` is pointing at my XDG directory. Because of such divergence, any operations that directly modifies the XDG directory is not reflected in the `.devbox/virtualenv` directory. However, the rest of the devbox CLI regards `.devbox/virtualenv` as the source of truth. This is the root cause of quite a few plugin errors such as mariadb, drupal, and python venv. ### Obscure XDG path The reason we have a hash directory in our XDG path is to avoid path collision. But since I use devbox so often, my XDG paths become a clown show: <img width="920" alt="Screenshot 2023-06-13 at 12 18 10 PM" src="https://github.com/jetpack-io/devbox/assets/2292093/b539362e-bfd7-4594-a7da-6bf76ad2e11c"> This makes it impossible to debug plugins. To get around the paths too long error described in #874, I override the env variable that points to the `.sock` path in our `run_test` scripts. ### TODO - [x] Clean up symlink creation functions - [x] Remove symlink tests ## How was it tested? ``` devbox run build cd examples/databases/mariadb devbox install devbox services up ```

savil force-pushed the savil/lepp-stack-fixes branch 2 times, most recently from 0557e19 to dcec06f Compare April 7, 2023 00:57

savil changed the title ~~use XDG runtime dir for postgres PGHOST to resolve "Unix-domain socket path is too long" error~~ [devbox quality] Harden postgres examples Apr 7, 2023

savil changed the title ~~[devbox quality] Harden postgres examples~~ [devbox] Harden postgres examples Apr 7, 2023

savil mentioned this pull request Apr 7, 2023

[example tests] generate "set -e" to exit scripts upon failure #869

Merged

1 task

savil force-pushed the savil/lepp-stack-fixes branch from bbb30db to 8014da0 Compare April 7, 2023 04:44

savil changed the base branch from main to savil/testscripts-homedir April 7, 2023 04:44

savil mentioned this pull request Apr 7, 2023

set HOME for testscripts #875

Merged

savil force-pushed the savil/lepp-stack-fixes branch 11 times, most recently from 785cb3c to f04d04e Compare April 7, 2023 21:42

savil changed the title ~~[devbox] Harden postgres examples~~ [devbox] Make stack-examples using Postgres actually work in CICD Apr 7, 2023

savil requested review from gcurtis and mikeland73 April 7, 2023 22:14

savil marked this pull request as ready for review April 7, 2023 22:14

savil changed the title ~~[devbox] Make stack-examples using Postgres actually work in CICD~~ [devbox] Make stack-examples using Postgres actually work Apr 7, 2023

mikeland73 requested changes Apr 7, 2023

View reviewed changes

savil mentioned this pull request Apr 7, 2023

[examples] enable stacks-django and rust examples to pass #880

Merged

savil commented Apr 7, 2023

View reviewed changes

savil force-pushed the savil/lepp-stack-fixes branch from b60ab66 to ccdce85 Compare April 8, 2023 00:35

savil requested a review from mikeland73 April 8, 2023 01:07

mikeland73 reviewed Apr 8, 2023

View reviewed changes

mikeland73 approved these changes Apr 8, 2023

View reviewed changes

Base automatically changed from savil/testscripts-homedir to main April 10, 2023 20:38

savil force-pushed the savil/lepp-stack-fixes branch 4 times, most recently from b3f465b to 783be5c Compare April 10, 2023 22:43

savil requested a review from mikeland73 April 10, 2023 22:44

gcurtis approved these changes Apr 11, 2023

View reviewed changes

savil added 3 commits April 11, 2023 16:02

use XDG runtime dir for postgres PGHOST to resolve "Unix-domain socke…

2b4c1ef

…t path is too long" error

review feedback: set XdgRuntimePath for package in Virtenv, remove en…

4e0c0e8

…sureVirtenvXYZ functions, use 5 length hash, and minor other changes

(1) use XDG_STATE_HOME (2) use createVirtenvSymlink and virtenvSymlin…

6749f20

…kPath (3) override XDG_STATE_HOME for examples testscripts

mikeland73 approved these changes Apr 11, 2023

View reviewed changes

use md5; undo rm PGDATA from lapp-stack devbox.json

9497fff

savil force-pushed the savil/lepp-stack-fixes branch from 783be5c to 9497fff Compare April 11, 2023 23:28

update symlink-test

b6f262f

savil merged commit a8cc1b4 into main Apr 11, 2023
9 checks passed

savil deleted the savil/lepp-stack-fixes branch April 11, 2023 23:57

LucilleH mentioned this pull request Jun 13, 2023

Remove virtualenv data xdg directory symlinks #1148

Merged

2 tasks

		"Virtenv": virtenvPathForPackage,
		"VirtenvRuntimePath": filepath.Join(virtenvXdgRuntimePath, pkg),

[devbox] Make stack-examples using Postgres actually work #874

[devbox] Make stack-examples using Postgres actually work #874

Conversation

savil commented Apr 7, 2023 • edited

Summary

Test Plan

savil commented Apr 7, 2023 • edited

mikeland73 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

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

savil left a comment • edited

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

savil Apr 7, 2023 • edited

Choose a reason for hiding this comment

savil commented Apr 8, 2023

mikeland73 commented Apr 8, 2023

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

mikeland73 left a comment

Choose a reason for hiding this comment

savil commented Apr 8, 2023

savil commented Apr 10, 2023

savil commented Apr 10, 2023 • edited

savil commented Apr 10, 2023

gcurtis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeland73 left a comment

Choose a reason for hiding this comment

savil commented Apr 7, 2023 •

edited

savil commented Apr 7, 2023 •

edited

savil left a comment •

edited

savil Apr 7, 2023 •

edited

savil commented Apr 10, 2023 •

edited