Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow remote shared cache testing from a network isolated sandbox #1498

Merged
merged 23 commits into from
Jan 5, 2024

Conversation

JakeSiFive
Copy link
Contributor

This is still very much a draft but it displays the basic components needed to get this idea to work.

Issues:

  1. Each test has to use a distinct database name manually right now, it would be nice if these were auto selected
  2. database names must be created prior to executing, it would be nice if the databases were created on the fly somehow
  3. Managing the workflow with vendoring is a bit odd right now, it would be nice if vendored dep settings could be bind mounted into place instead
  4. A non-rustup version of cargo has to be used to build this. I managed to get this working locally via a local install of a non-rustup toolchain. This might get a bit annoying if we don't have a way to download the version of cargo/rust that you're meant to build with
  5. Right now things are just in the state I first got them working in, it would be nice if we could have a generic mechanism for running RSC commands in these environments

@JakeSiFive
Copy link
Contributor Author

ok almost done, I'm gonna get these tests running in CI as the last step and then I'll take this off of draft. Go ahead and review the current form of it now because I only plan on adding code, not making modifications outside of review requests now.

@JakeSiFive JakeSiFive marked this pull request as ready for review January 4, 2024 18:10
Comment on lines +112 to +115
shim_db
.execute_unprepared(&format!("CREATE DATABASE {}", db))
.await?;
drop(shim_db);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what drop is doing here/why it is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think its needed but it causes the shim_db to be droped before we create a new one so that we don't have two connections at once.

It also might help enforce that the follow up connection uses the same view version of the databases as the one we just created (e.g. drop might wait for transactions to be committed but I'm not too sure about that and it shouldn't be needed strictly here, I think the await is enough to know that the commit has happened and any new connections will see the new database.

Some dir -> Pass "{dir}/bin"
None ->
require Some postgresPath = whichInEnvPath "postgres"
else failWithError "could not find postges on path"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: postges -> postgres

@@ -80,6 +80,9 @@ test: wake.db
unittest: all
$(WAKE_ENV) ./bin/wake --in test_wake runUnitTests

remoteCacheTests: all
$(WAKE_ENV) ./bin/wake -d -x 'testPostgres Unit'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Do we need -d here? Also should testPostgres be in test_wake like the other tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-d It makes it a million times

Also I'm not going to mess with test_wake stuff right now since thats its own bag of worms

@@ -1,6 +1,6 @@
FROM alpine:3.14.0

RUN apk add m4 g++ make pkgconf git tar xz gmp-dev re2-dev sqlite-dev fuse-dev ncurses-dev dash sqlite-static ncurses-static linux-headers jq
RUN apk add m4 g++ make pkgconf git tar xz gmp-dev re2-dev sqlite-dev fuse-dev ncurses-dev dash sqlite-static ncurses-static linux-headers jq postgresql
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are building manually all these should be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a lot of them are needed, just not the postgres dep

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that is what I meant lol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"all these" -> postgres from all these different files not the entire dep list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh gotcha, I'm going to remove postgres testing from the other deps for now

@@ -0,0 +1,43 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be deleted if we are generated the spec file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, I just haven't gotten around to it, hold your horses on clean up comments.

Comment on lines 39 to 40
- name: Vendor remote cache deps
run: cd rust/rsc && cargo vendor --locked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you deleted too much

@JakeSiFive JakeSiFive merged commit f2aed8e into master Jan 5, 2024
12 checks passed
@JakeSiFive JakeSiFive deleted the wakebox_env_for_postgres_testing branch January 5, 2024 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants