Skip to content
This repository was archived by the owner on Oct 4, 2022. It is now read-only.

Demo v2 Sketch#95

Merged
sipsma merged 16 commits into
dagger:mainfrom
sipsma:workflow-clean
Sep 1, 2022
Merged

Demo v2 Sketch#95
sipsma merged 16 commits into
dagger:mainfrom
sipsma:workflow-clean

Conversation

@sipsma
Copy link
Copy Markdown
Contributor

@sipsma sipsma commented Aug 22, 2022

Working on the features and implementation outline of https://github.com/dagger/cloak/blob/main/demos/v2/demo-v2.md

Demo P1

  • Ability to use docker credentials to pull images
  • Anything that can make errors more bearable (truncate filesystem IDs)
  • Collect types provided back to caller in go Engine API into a single struct
  • Increase timeout of nodejs sdk polling for engine to be ready (it's 60 seconds now, but no harm in increasing in case slow internet during demo or something)

Demo P2

  • Pull image config, apply to execops
    • Less straightforward than expected, defering for later
  • Fix naming of DeployNetlifyDeploy object (maybe call the type URLs or something)
    • Updated name of the graphql type from Deploy to SiteURLs, but the client model codegen somehow decided to then make the struct named DeployNetlifyDeploySiteURLs rather than DeployNetlifySiteURLs (???). So made this update but it didn't help much.
  • Extend Filesystem from yarn (for nicer chains)

Before merge into main

  • Egregious hacks where .ssh/known_hosts is overwritten or mounted over in containers (especially when done in every execop)
  • Decide on how to package nodejs sdk (NodeJS SDK: decide how to package dagger#3096)
  • At least fix naming of --workdir, -p files+types in extension/ etc. Think again if there's any short term changes to make to the actual implementation (look at magefile)
  • Update docs
    • Don't forget that updating .gitconfig is still a pre-req for now if writing an out-of-tree go extension

Maybe, but probably just create backlog issue

  • "find-up" behavior for cloak projects

@sipsma sipsma force-pushed the workflow-clean branch 3 times, most recently from bfd9bde to f1d2caa Compare August 22, 2022 21:43
@sipsma
Copy link
Copy Markdown
Contributor Author

sipsma commented Aug 22, 2022

@shykes @samalba @aluzzardi End of last week we decided to go with workflows being run on the operator host, but didn't discuss in depth what changes there'd be to cloak.yaml as a result. So for now I just went with what felt best, it's not all that different than what we've discussed in the past, but want to run it by you before merging. Doc updates here: f1d2caa

Basically:

  • Workflows don't need to be declared at all in cloak.yaml
  • Top level dependencies key specifies other cloak.yamls containing extensions that you want loaded when your workflows and/or extensions executes.
  • Extensions for the project are declared under the sources key. In retrospect (as I type this) maybe this should be called extensions? Let me know what you think, trivial to update.

Also, I think that for case where you only want to workflows, no extensions, it shouldn't be necessary to write a separate cloak.yaml at all; you should instead be able to just provide the dependencies as an argument to the Engine library you import. cloak.yaml would thus only be needed when defining a project containing extensions. I will save this for a follow-up though as it requires another tweak to the loadExtension API (basically, there has to be the current version that extends Filesystem plus another version that just accepts the input parameters present in the yaml as a graphql input object).
EDIT: we discussed this and other issues live, there are still many cases where even when you only have a workflow you'd want an explicit cloak.yaml (i.e. you want to play with cloak dev, you want to generate client stubs for your dependencies), so I think we can safely ignore skipping cloak.yaml for now. The cases where it could be nice are too small a percentage to care about now.

@sipsma
Copy link
Copy Markdown
Contributor Author

sipsma commented Aug 23, 2022

There are still some issues we are figuring out based on discussion earlier, but I want some of the changes here to not be blocked (i.e. removal of Dockerfile boilerplate), so I merged the commits except for the initial todoapp workflow and the update to the docs related to how workflows are configured. Will leave these as a PR until we get consensus in the next day or two in case anyone has any comments.

@sipsma sipsma changed the title Initial support for TS workflows [WIP] Demo v2 Sketch Aug 23, 2022
@sipsma sipsma force-pushed the workflow-clean branch 5 times, most recently from 5169339 to 220d40b Compare August 24, 2022 04:03
@sipsma
Copy link
Copy Markdown
Contributor Author

sipsma commented Aug 24, 2022

Update for today:

  1. Git+SSH auth support
    • Support for git deps in cloak.yaml (which uses SSH_AUTH_SOCK as needed for ssh creds) is merged into main
    • Support for specifying git+ssh deps in package.json is in this PR. I have also tested using go mod deps from private git repos on the host. More details on all that below
  2. Support for {host{workdir{read}}} and {host{workdir{write}}} is in this PR (code needs cleanup before merge to main)
  3. Actual "build" workflow works now, included in this PR. Is the version that uses yarn (the easy way).
    • At the moment this is under the cloaktest script in todoapp's package.json, just because I didn't want to deal with how to rename the existing build script that calls react-scripts (have multiple ideas here, just saving it for later)
  4. Have a preliminary deploy workflow in Go, made some small updates to cloak generate to get the stub generation working.
    • More updates to the go sdk's engine.Start needed to get this fully working, but it does run and missing stuff is pretty minor (like automatically loading dependencies whereas existing uses of engine.Start under cmd/cloak do that manually)

Notes (light on details here to save time, mostly just listing these for my own memory for discussion in live meeting tomorrow):

  1. In this PR I got rid of the separate package.json for the workflow and just re-use todoapp's existing one. Many reasons for this as mentioned by Andrea earlier today, but I can confirm it ends up being much simpler and feels better.
  2. I am temporarily avoiding some of the TS related issues right now by just writing the workflow in js. It can still import our TS SDK easily and given how short it is, I don't miss the type-checking of TS all that much.
  3. I am trying the workflow-centric approach to see how smooth I can get it. Some hard parts:
    • The js workflow needs to tell cloak engine where the directory holding the project file is. The file is right next to the source code for the workflow, but that dir is not the cwd when the workflow is invoked, so there's a weird trick I have to do to get the directory of the source file (which also holds cloak.yaml)
      • One bad part of this is that its boilerplate that must exist in the user's workflow code (can't exist in the SDK)
    • go run doesn't work easily, weird errors when putting "deploy": "go run $(pwd)/workflows/deploy" in package.json. Believe it's because go.mod is in workflows/deploy but go command sees that its in a subdir of an existing go module (the actual cloak repo's go.mod) and gets confused (something about submodules).
      • For now, deal with this by doing cd workflows/deploy && go run ., which works. But then I need to use the CLOAK_WORKDIR trick, which is not hard to implement but just very confusing as a user.
      • I think a better approach might be to put go.mod next to package.json in the todoapp repo root, will try tomorrow.
    • Overall, I am leaning more towards the project-centric approach given the complication w/ the build workflow that I think would disappear if cloak.yaml was set for the whole project (todoapp). But still worth more discussion and experimenting.
  4. Git+SSH Auth
    • If SSH_AUTH_SOCK env is set when invoking cloak and/or a workflow, then engine will make that sock accessible to any git operations, including the new support for git deps in cloak.yaml.
      • Worth considering whether this automatic behavior is good, maybe it should be opt-in?
    • The issue mentioned here: Support for specifying dependencies from git url #100 (comment)
      • You can currently see in todoapp's package.json that I'm using "@dagger.io/dagger": "git+ssh://git@github.com:sipsma/cloak-nodejs-sdk.git#main", this is my very temporary test of the idea there where I've created a separate repo that has just the nodejs sdk and nothing else. Obviously we can't use my own private repo during the demo, but we could consider making one under the dagger org. Or maybe there's something simpler.
    • The deploy workflow has it's own go.mod, which relies on github.com/dagger/cloak, which is a private repo. I got this working with my SSH_AUTH_SOCK, but go's toolchain makes this incredibly obnoxious. Have to set GOPRIVATE env and also update my .gitconfig with a url.insteadOf setting to remap https->ssh. Will need to think about how this will work w/ extensions where that all needs to happen inside containers.
    • In this commit in the PR I temporarily updated the exec implementation to always mount the ssh auth socket into every execop. Even worse, in order to verify github's ssh keys I also mount a known_hosts into every execop. This is obviously a complete disgusting hack; might be okay for a demo but need better approach before merging into main.

Comment thread core/core.schema.go Outdated
Comment thread core/core.schema.go
}
}
}
query Workdir() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For later: API design:

  • I think we should avoid having a "gRPC-like" API over GraphQL (e.g. imperative procedures like "read" and "write"). The GraphQL "native" way (not that I'm a reference here, just based on the APIs I saw) would probably be a top-level workdir(optionalThingsSuchAsIncludeExclude: String): Filesystem! for read
  • As for write, it should either be a top-level mutation (writeFilesystemToHost(input: AnInputType!))
  • Or better yet, (but harder to do), Filesystem should have a contents: String (or probably contents: Stream) and it's a SDK concern to grab the contents and de-serialize to the host (just like buildkit does). So the user query would be { yarn { build { fs { contents } } } and unpack locally

Given the timeline, it's ok to postpone even having this conversation until after the demo (unless there's some easy changes that get us close to that)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Didn't address this one as part of this PR, can tackle this as part of API refactoring

@sipsma
Copy link
Copy Markdown
Contributor Author

sipsma commented Aug 26, 2022

Pushed a fix for the issue David ran into with TS extensions not building the nodejs sdk correctly (handle both local and remote deps on that seamlessly now).

Gonna go automate the go.mod insanity via cloak generate next.

@sipsma sipsma force-pushed the workflow-clean branch 6 times, most recently from e562086 to 7f73403 Compare August 29, 2022 16:06
@sipsma sipsma force-pushed the workflow-clean branch 2 times, most recently from 6936aaa to b9c5efc Compare August 29, 2022 17:25
@sipsma sipsma force-pushed the workflow-clean branch 5 times, most recently from 129c12a to c4093fe Compare September 1, 2022 02:28
sipsma added 3 commits August 31, 2022 19:29
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
@sipsma sipsma force-pushed the workflow-clean branch 4 times, most recently from e81c859 to 8705d14 Compare September 1, 2022 03:14
@sipsma sipsma changed the title [WIP] Demo v2 Sketch Demo v2 Sketch Sep 1, 2022
sipsma added 13 commits August 31, 2022 20:42
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
@sipsma
Copy link
Copy Markdown
Contributor Author

sipsma commented Sep 1, 2022

Merging as is in order to reduce pain of everyone basing off my branch.

  • There are still some hacks in here, but the worst ones are gone (I will be able to sleep at night) and I'll create issues for the immediately actionable fixups right after merge. There are also many open questions about projects/extensions/scripts, but it's not worth blocking this on figuring those out.
  • Getting started docs are also updated with the new and improved DX, tested them.
  • Also tested the todoapp demov2 repo, everything is working there against the today's updates here. I have left the demov2 tag alone for now. Will send out an update for todoapp (tweaks like renaming workflows->`scripts) right after this too

@sipsma sipsma merged commit 2894044 into dagger:main Sep 1, 2022
@@ -0,0 +1,20 @@
import { GraphQLClient } from "graphql-request";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need to commit compiled files?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants