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

First class dagger shell #736

Merged
merged 17 commits into from
Feb 6, 2025
Merged

First class dagger shell #736

merged 17 commits into from
Feb 6, 2025

Conversation

sourishkrout
Copy link
Member

@sourishkrout sourishkrout commented Feb 5, 2025

In order to natively support Dagger's shell Runme needs to take the entire notebook as opposed to an atomic cell into account. This PR introduces the APIs necessary to transform a notebook into a Dagger Shell script using some basic AST transformation.

Integration with extension in stateful/vscode-runme#1947.

PS: The protos are not published yet and need local-gen to work.

The new service will turn a deserialized version of the following markdown into a runnable Dagger shell script.

Simple example notebook

Unamed cell:

git github.com/stateful/runme |
    head |
    tree |
    file examples/README.md

And the same as a named cell:

# Named in README in cell attributes
git github.com/stateful/runme |
    head |
    tree |
    file examples/README.md

Resulting Dagger Shell Script

Running the unamed cell:

DAGGER_01JKE40B6A0F5NBESE49X122RD()
{
  git github.com/stateful/runme \
    | head \
    | tree \
    | file examples/README.md
}
DAGGER_01JKE40B6A0F5NBESE49X122RD

Running the named cell:

DAGGER_01JKE4K781F87XRK9SQZ9T1GQD()
{
  git github.com/stateful/runme \
    | head \
    | tree \
    | file examples/README.md
}
README()
{
  git github.com/stateful/runme | head | tree | file examples/README.md
}
README

Copy link
Collaborator

@adambabik adambabik left a comment

Choose a reason for hiding this comment

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

A couple of suggestions, but overall LGTM.

One more asks I have is to add an example in the PR description of the input (exemplary Markdown) and the expected output (a script) and how it will be executed.

service NotebookService {
// ResolveNotebook that takes a notebook and a cellIndex and returns a script
// that takes the whole notebook into account to run as cell.
rpc ResolveNotebook(ResolveNotebookRequest) returns (ResolveNotebookResponse) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Is there any reason this method couldn't be in RunnerService?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's where I had it first. I decided to make it a separate service because the Runner only knows about atomic cells, and this service needs the entire notebook. Perhaps that's not "different enough". We could consider bringing it back into the Runner as part of runnerv2.

internal/notebook/service.go Outdated Show resolved Hide resolved
internal/notebook/resolver.go Outdated Show resolved Hide resolved
internal/notebook/resolver.go Outdated Show resolved Hide resolved
internal/notebook/resolver.go Outdated Show resolved Hide resolved
@sourishkrout sourishkrout merged commit f192d68 into main Feb 6, 2025
8 checks passed
@sourishkrout sourishkrout deleted the seb/first-class-dagger-shell branch February 6, 2025 17: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