add support for linking remote instances and projects initialization

[nsidnev]

This PR adds support for edgedb project and edgedb instance link in action! New inputs for this action try to mimic existing CLI flags and options where it's possible. I also added several examples of using the action in the README.

Unfortunately, I wasn't able to successfully test it on real project as this action installs CLI version from August (link to index of nightly packages), and because of this edgedb project init --link exits with an error. I also looked at the nightly builds workflow in the EdgeDB CLI and it seems that they all completed successfully. Could it be that the package index requires manual updating?

Example of failed workflow (with debug logs if necessary): https://github.com/nsidnev/edgedb-elixir/runs/3617122318#step:5:46

Refs #11

[nsidnev]

@elprans friendly ping :)

[nsidnev]

It looks like the package index has been updated and it now includes the new CLI version, so after restarting the workflow everything went well!

Workflow file
Passed workflow run

PS. The explicit server-version: none argument doesn't seem very convenient :( Maybe it can be removed after release of new major version of this action?

[tailhook]

My main concern is that this mostly directed to using services for edgedb. But this is a bit more annoying than what we want. I think it should work along the lines of:

1. If project-link: "edgedb://localhost:5656/" is specified, it overrides everything below and just links the instance (no server install, no instance creation)
2. If server-instance is specified create that named instance (with server-version or stable) and link it as a project
3. With no params and existing edgedb.toml it should run project init
4. With server-version: 1-beta4 and edgedb.toml it should override server version project init --server-version=1-beta4 (this is "compatibility check mode" for CI)
5. With no edgedb.toml it should install stable or specified version of edgedb (same as now)

So example with service look like:

jobs:
  test:
    runs-on: ubuntu-latest
    name: CI with EdgeDB CLI
    services:
      edgedb:
        image: edgedb/edgedb:1-beta3
        env:
          EDGEDB_SERVER_INSECURE_DEV_MODE: 1
        ports:
          - 5656:5656
    steps:
      - uses: actions/checkout@v2
      - uses: edgedb/setup-edgedb@v1
        with:
          project-link: edgedb://localhost:5656
      - run: edgedb query "SELECT 'Hello from GitHub Actions!'"

(None: insecure dev mode allows empty password and generated self-signed cert, and project-link with DSN allows connecting to remote instances too)

And we have project-init: false to suppress initing/linking a project if edgedb.toml is existing or server-instance is specified.

But I defer final judgement to @elprans

[tailhook]

This linking by default is probably not backwards-compatible?

[elprans]

I think it's fine. We can call this setup-edgedb@V2.

[tailhook]

Perhaps we should include an example without server-version: none? Our premise is that most people would use server installation by default and don't bother with writing their own "services" definition. Although, in that case we should use a version from edgedb.toml rather than latest stable.

[elprans]

Yes. The recommended and default way to use EdgeDB in CI is to use the instance set up by the action. Using "services" is an advanced pattern that, I think, only makes sense if you want to use a custom EdgeDB server image.

[nsidnev]

@tailhook thanks for review! I'll change PR to apply your suggestions and change the examples so that they are more focused on using the server provided by action instead of services, but I have some questions.

1. If project-link: "edgedb://localhost:5656/" is specified, it overrides everything below and just links the instance (no server install, no instance creation)

Should this argument create a random instance name for instance/project linking process? IMO it's a bit confusing if we have a server-instance argument that will always create a new instance.
Maybe server-instance should create an instance link to the server pointed by project-link if project-link is valid, and create a new instance only if project-link is "false"? If this is the case, then it also implies for us that the project-link must have a constraint at which step with action will fail if input argument server-instance is skipped. What do you think about this?

(None: insecure dev mode allows empty password and generated self-signed cert, and project-link with DSN allows connecting to remote instances too)

I've checked out some PRs related to this change, but right now I haven't been able to check how I can work with it.

1. latest docker image uploaded to Docker Hub doesn't seem to support this environment variable. I also tried this with tags 1-rc1 and nightly. It looks like support for this isn't implemented in edgedb/edgedb-docker:

 docker run --rm -it -p 5656:5656 -e EDGEDB_SERVER_INSECURE_DEV_MODE=1 -e EDGEDB_SERVER_PASSWORD=edgedb edgedb/edgedb:1-rc1
Bootstrapping EdgeDB instance on the local volume...
=======================================================================
                                 ERROR
                                 -----

EdgeDB server requires a TLS certificate and a corresponding private
key to operate.  You can either provide them by setting the
EDGEDB_SERVER_TLS_CERT_FILE and EDGEDB_SERVER_TLS_KEY_FILE environment
variables, or set EDGEDB_SERVER_GENERATE_SELF_SIGNED_CERT to generate
the certificate automatically.

Example:

  docker run -e EDGEDB_SERVER_GENERATE_SELF_SIGNED_CERT=1 edgedb/edgedb
=======================================================================

2. Then I built the EdgeDB server from the master and started it with EDGEDB_SERVER_INSECURE_DEV_MODE=1 edb server, but the latest CLI, also built from master, raises an error without the --trust-tls-cert option:

EDGEDB_INSECURE_DEV_MODE=1 cargo run instance link --non-interactive --dsn edgedb://edgedb@localhost/edgedb tmp
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
     Running `target/debug/edgedb instance link --non-interactive --dsn 'edgedb://edgedb@localhost/edgedb' tmp`
Authenticating to edgedb://edgedb@localhost:5656/edgedb
[2021-09-30T20:28:46Z WARN  rustls::session] Sending fatal alert BadCertificate
edgedb error: ClientConnectionFailedError: invalid certificate: UnknownIssuer

I'm not sure what I'm doing wrong, could you help me with this, please?

[elprans]

Support for EDGEDB_SERVER_INSECURE_DEV_MODE is added in geldata/gel-docker#31. I'll rebuild the images once that lands.

[tailhook]

1. If project-link: "edgedb://localhost:5656/" is specified, it overrides everything below and just links the instance (no server install, no instance creation)

Should this argument create a random instance name for instance/project linking process? IMO it's a bit confusing if we have a server-instance argument that will always create a new instance. Maybe server-instance should create an instance link to the server pointed by project-link if project-link is valid, and create a new instance only if project-link is "false"? If this is the case, then it also implies for us that the project-link must have a constraint at which step with action will fail if input argument server-instance is skipped. What do you think about this?

For random instance name -- probably. We have some default instance name for interactive project init but, it can't be used from the cli. So random would be good enough. It should be ghactions_3748 or something, not too random like uuid.

As for server-instance it sounds like should create a new instance. We can probably rename it into an instance-name and do something like this, if you want nicely named instances, let's reformulate it as:

1. If project-link: "edgedb://localhost:5656/" is specified, it just links the instance (no server install, no instance creation)
2. Otherwise, if edgedb.toml exists it should run project init
3. Otherwise, if instance-name is specified it should create and link instance named instance-name
4. Otherwise, just it should just install EdgeDB (same as now)

In (2), (3), (4) you can override server-version. In (1), (2) we use random name which could be overriden by instance-name.

So project-link always suppresses installation and instance creation. And to have an instance you have either having edgedb.toml or instance-name in config (with most setups should fit former).

Sounds like we should also have cli-only: true flag for skipping installation without project-link. And drop server-version: none as it only makes sense for (4) and doesn't play nice with (3) and confusing with (2).

right now I haven't been able to check how I can work with it

Ah, yes sorry. I've assumed that it would work because it's supported by server itself. But we have more logic in the container itself, so as @elprans said it will land soon.

but the latest CLI so built from master, raises an error without the --trust-tls-cert option

Yes. Insecure dev mode for server only ignores the password checking. So yes --trust-tls-cert is needed (you can also use EDGEDB_INSECURE_DEV_MODE in the client, but that should be there on every invocation rather than on link stage, so not what we want here).

[elprans]

Sounds like we should also have cli-only: true flag for skipping installation without project-link.

What's the use case for this?

[tailhook]

Sounds like we should also have cli-only: true flag for skipping installation without project-link.

What's the use case for this?

Any use case that we haven't covered yet: edgedb.toml in subdirectory, or mulitple ones, using edgedb-server --temp-dir, or working with (multiple) remote locations, etc.

[elprans]

Any use case that we haven't covered yet

OK, but the server installation in that case would be unnecessary, but not harmful, so what's the benefit of adding the option?

[nsidnev]

I've updated PR and implemented most of the suggestions (except for the cli-only option, because I also don't understand why we need it). But right now, this PR can only link remote instances with project, but not initialize new projects or create separate named instances using server provided by this action.

This is due to an error that EdgeDB raises in the GitHub Actions environment when bootstrap process starts:

##[debug]CRITICAL 5307 2021-10-03T23:31:59.476 edb.server: cannot create the runstate directory: '/run/user/1001' does not exist; please use --runstate-dir to specify the correct location

If I understood correctly, then right now the workflows for JS and the Go drivers are starting server manually with the --temp-dir option, and because of this they are not getting any errors. But there are no such options in the CLI (or I missed them), so I'm not sure what to do in this case. One way to handle this case would be to add new options to CLI, but wouldn't that make project init/instance create commands overcomplicated?

Workflow for EdgeDB running in services: https://github.com/nsidnev/edgedb-elixir/runs/3785044417
Workflow file. Don't look at the "failed" status here, I've slightly changed configuration of roles for this branch, and because of this, some of my authentication tests failed :)

Workflow for EdgeDB created by project initialization with link to error line: https://github.com/nsidnev/edgedb-elixir/runs/3785060819#step:4:147
Workflow file

[tailhook]

OK, but the server installation in that case would be unnecessary, but not harmful, so what's the benefit of adding the option?

Speed of github action. We can wait for someone to ask, though.

One way to handle this case would be to add new options to CLI, but wouldn't that make project init/instance create commands overcomplicated?

The problem with that directory is not the directory per se, but that it means that there are no systemd --user daemon running. So we probably have to run server as foreground, and daemonize it manually:

XDG_RUNTIME_DIR=${TMPDIR}/edgedb edgedb instance start $NAME --foreground &

And to have bootstrapping worked we have to pass same environment variable to project init:

XDG_RUNTIME_DIR=${TMPDIR}/edgedb edgedb project init 

And then check that exit status is 2 (which means bootstrap and migrations are okay, but could not start service).

Technically we can incorporate this to the CLI tool. But I'm not sure in what form. Because on actual user's machines presence of systemd daemon can depend on way they have been login, and maybe on other things.

Some brainstorm on what we might do:

1. edgedb project init --start-conf=manual (we already have this option in instance create), which will just avoid checking for error code 2
2. edgedb project init --ci-mode which will background the server (and perhaps move XDG_RUNTIME_DIR)
3. Or maybe spawn systemd --user daemon in this action

[tailhook]

Good work! I've let some tiny comments, but let's settle on the project init (as of comment above).

[tailhook]

Looks like this will no longer work as none equals to non-specifying the version at all.

[nsidnev]

If you omit server-version in the workflow file, actions will use "stable" as default value as it was before. I haven't changed this behavior from previous version. As a result, first example installs both CLI and server, while this one only installs the CLI. I think that's the equivalent of the cli-only option you mentioned about above, no?

[tailhook]

Suggested change

	using `egedb project init` and `edged.toml` file exists in the repository.
	using `egedb project init` and `edgedb.toml` file exists in the repository.

[tailhook]

I think you can put instance-name: ci_edgedb_instance # optional in the example above. Separate example makes only scrolling longer.

[tailhook]

We probably have to put a better comment on the first example, explaining that it's behavior depends on edgedb.toml, because they are the same and user has to carefully compare yaml contents and comments to figure out that.

[nsidnev]

Added more explanations of behavior of the action in the README and to the first example

[nsidnev]

Some brainstorm on what we might do:

1. edgedb project init --start-conf=manual (we already have this option in instance create), which will just avoid checking for error code 2
2. edgedb project init --ci-mode which will background the server (and perhaps move XDG_RUNTIME_DIR)
3. Or maybe spawn systemd --user daemon in this action

I think the first option is quite good, I like it. It doesn't look like it would be hard to implement, and it's similar to existing CLI command. This method can also be useful for adding support to systems that won't have systemd.

[nsidnev]

@tailhook thanks for implementing --server-start-conf! I just updated this PR and now edgedb project init using the local EdgeDB server works! 2 things that may need additional review:

1. I added output for action, if the users want to handle similar stuff manually, then they can use the output value of runstate-dir. I think this might be useful, but if needed, I can remove it
2. Since @actions/exec doesn't support spawning processes in the background, I implemented this using the raw child_process from Node

[nsidnev]

@tailhook @elprans is there anything else I can do to get this PR merged? I already use this branch in my project and everything works, but using the original action would be preferable.

[nsidnev]

friendly ping :)

[elprans]

Looks good to me! @fantix, please take a quick look too.

[fantix]

This is very cool!

I think it's not working properly with CLI 1-rc1 (missing the edgedb project init --server-start-conf option), but nightly works fine.

[nsidnev]

Congratulations on the release of the new EdgeDB RC! I think now, when edgedb-cli includes the --server-start-conf option, this PR can be safely merged, right? :)

Just in case, to make sure everything will work correctly, I've created a new branch that will include an update to EdgeDB RC2, which will use this action with the latest stable version of the CLI. I haven't updated the driver code right now, but the current workflow with the latest stable version of the CLI works!

Workflow file: link.
Passed workflow: link.

[tailhook]

Thanks, @nsidnev! I've also just merged CLI with the portable distribution installation method by default. So I think we should ensure that it works as well on the new nightly (will be released tomorrow).

[nsidnev]

@tailhook I updated the workflow to use the nightly CLI and ran into serveral problems with new installation method:

1. I saw that the CLI now recommends using the 1.0-rc.2 format, so I changed it in the edgedb.toml file and tried to use it in the workflow file as well. Unfortunately, I got an error that the version could not be found. So now the action successfully installs the server and the CLI when 1-rc2 is used as the action input and 1.0-rc.2 is used in the edgedb.toml file.
   Workflow run when using 1.0-rc.2 in both edgedb.toml and tests.yml: https://github.com/nsidnev/edgedb-elixir/runs/4227535923
   Workflow run when using 1.0-rc.2 in edgedb.toml and 1-rc2 in the tests.yml workflow: https://github.com/nsidnev/edgedb-elixir/runs/4227224528

2. There is a coroutine leak for _signal_sysevent, although I'm not sure if this has already been fixed in the server's master: https://github.com/nsidnev/edgedb-elixir/runs/4227224528?check_suite_focus=true#step:3:167

3. The CLI panicked when it tried to restart the systemd daemon, even though it used the --server-start-conf=manual flag when initializing the project: https://github.com/nsidnev/edgedb-elixir/runs/4227224528?check_suite_focus=true#step:3:170

[tailhook]

@tailhook I updated the workflow to use the nightly CLI and ran into serveral problems with new installation method:

1. I saw that the CLI now recommends using the `1.0-rc.2` format, so I changed it in the `edgedb.toml` file and tried to use it in the workflow file as well. Unfortunately, I got an error that the version could not be found. So now the action successfully installs the server and the CLI when `1-rc2` is used as the action input and `1.0-rc.2` is used in the `edgedb.toml` file.
   Workflow run when using `1.0-rc.2` in both `edgedb.toml` and `tests.yml`: https://github.com/nsidnev/edgedb-elixir/runs/4227535923
   Workflow run when using `1.0-rc.2` in `edgedb.toml` and `1-rc2` in the `tests.yml` workflow: https://github.com/nsidnev/edgedb-elixir/runs/4227224528

I see --method=package in the log. This means old installation. We will be removing --method in a month or so.

3. The CLI panicked when it tried to restart the systemd daemon, even though it used the `--server-start-conf=manual` flag when initializing the project: https://github.com/nsidnev/edgedb-elixir/runs/4227224528?check_suite_focus=true#step:3:170

Ah, yes, sorry. Will fix that todo tomorrow.

[nsidnev]

Just checked the action with the latest nightly CLI release. Everything works perfectly!

Passed workflow: link

[nsidnev]

@tailhook I added new jobs to the test workflow to test new behavior and found 2 new error cases, the first of which could break existing projects after merging this PR:

1. The nightly CLI now terminates with an error when executing the edgedb server info --bin-path command, but works if I add the --latest option (in case there are no --nightly or --version options). But if I add it, job will fail on the current stable release of the CLI, because that option doesn't exist there. This is the case when stable is used as the server-version input for the action.
2. The integration with projects also doesn't work on Mac OS. I'm not very familiar with it and don't have any device with it, so I can't say exactly why it can't find packages.

These jobs are slightly modified examples from the README, and I can try to add them to this branch, but I'm not sure it's possible right now to make all jobs pass succesfully, at least because edgedb server install doesn't work on the stable release with --version=1.0-rc.2 option.

I also forgot that this action works on windows, so this PR won't work with it. I can try to add support for it, but I'd rather do it in a separate PR than this one.

Workflow file: https://github.com/nsidnev/setup-edgedb/blob/3c3061f1aa7c86553a90219521c0a5d453e3032e/.github/workflows/test.yml
Workflow run: https://github.com/nsidnev/setup-edgedb/actions/runs/1488078429

[tailhook]

@tailhook I added new jobs to the test workflow to test new behavior and found 2 new error cases, the first of which could break existing projects after merging this PR:

1. The nightly CLI now terminates with an error when executing the `edgedb server info --bin-path` command, but works if I add the `--latest` option (in case there are no `--nightly` or `--version` options). But if I add it, job will fail on the current stable release of the CLI, because that option doesn't exist there. This is the case when `stable` is used as the `server-version` input for the action.

Yes. This is done intentionally since server info without version selector is not consistent. It's not "last version installed" (there are no logs, it doesn't work on nightly, etc). latest vs nightly vs version selector is all around our coomman-line, so should be good enough.

Yes this is breaking change, and we think the scope things that will break is minimal before 1.0.

2. The integration with projects also doesn't work on Mac OS. I'm not very familiar with it and don't have any device with it, so I can't say exactly why it can't find packages.

These jobs are slightly modified examples from the README, and I can try to add them to this branch, but I'm not sure it's possible right now to make all jobs pass succesfully, at least because edgedb server install doesn't work on the stable release with --version=1.0-rc.2 option.

We'll release a new stable of CLI shortly. And the issue with runtime dirs I'm fixing in geldata/gel-cli#560

I also forgot that this action works on windows, so this PR won't work with it. I can try to add support for it, but I'd rather do it in a separate PR than this one.

We are dropping windows support for server/instance/project commands in CLI and will work on that later. For now if someone needs windows they will have to use docker image manually. I mean setup-edgedb will not support windows for some time now (we should fix it in a month or so).

[elprans]

I mean setup-edgedb will not support windows for some time now

setup-edgedb@v1 does not rely on server or instance commands on Windows. Instead it performs manual installation by running Linux server install via wsl, so at least the installation should continue to work.

[nsidnev]

We'll release a new stable of CLI shortly.

So can I already change the command in this branch to always use the selector? Also, should I add new workflow jobs to this PR or is this something unnecessary right now?

[tailhook]

We'll release a new stable of CLI shortly.

So can I already change the command in this branch to always use the selector?

Yes.

Also, should I add new workflow jobs to this PR or is this something unnecessary right now?

I'm not sure I understand the question.

[nsidnev]

Yes.

Done. Right now everything except the stable CLI works fine!

I'm not sure I understand the question.

Sorry. I added new jobs to the test.yml workflow that test the changes in this branch. It tests changes for different OS, CLI and server combinations. I think it's reasonable to keep new jobs.

[elprans]

CI is green with rc3 CLI up. I took another look at the option names and decided that project-link should instead be server-dsn. I also made project initialization on a linked instance conditional on edgedb.toml like the non-linked branch.

[elprans]

Thank you very much for the contribution, @nsidnev. Great work!

[nsidnev]

CI is green with rc3 CLI up

Awesome! Thanks for releasing new version, I've already changed my projects to use the new tag from original action.

Great work!

Thanks, I was happy to help with that :)