Mostly flagless app source by itowlson · Pull Request #1179 · spinframework/spin

itowlson · 2023-02-19T23:43:01Z

This is similar to #1159 but compromises on total flaglessness to make it play nicely with trigger options. With this, you still have to write -f for anything other than the default source (current directory + spin.toml), but you don't need to write --from-file or --from-registry unless the "inference engine" gets it wrong. E.g.

# Infers: File: ./spin.toml
spin up

# Infers: File: ./app.toml
spin up -f app.toml

# Infers: File: ./guest/spin.toml
spin up -f guest

# Infers: Registry: ghcr.io/test/myapp:v1
spin up -f ghcr.io/test/myapp:v1

Error cases:

# If not a file/directory
$ spin up -f guest2
Error: File or directory 'guest2' not found. If you meant to load from a registry, use the `--from-registry` option.

# If not a file/directory, *but* something that looks like a tag is present
$ spin up -f guest2/bish/bosh:v1
Error: cannot pull Spin application from registry

Caused by:
    Not authorized: url https://index.docker.io/v2/guest2/bish/bosh/manifests/v1

The following tweaks are also made to the spin up UI:

Bindle flags are hidden but continue to work (no deprecation warning is issued)
The from-bindle alias is added despite this, because consistency even on the edge of the pit
--file is moved to an alias but continues to be supported

There should be no breaking change for the -f flag, except that if it is pointed at a non-existent file, it may now fail with a registry error instead of a file not found error.

NOTE: this currently has the full, tragic commit history of the flagless work; if we decide to adopt it, then this should be squashed before merging.

itowlson · 2023-02-20T02:24:06Z

I would really really appreciate it if someone with access to some more realistic registry references could test this - I have been testing with one of Brian's that I don't have access to and confirmed that it gives a suitable OCI error, but it would be good to get more variation especially since the reference parsing involves heuristics.

(Hark at me, first inference engine and now heuristics! It's a couple of if expressions Ivan, don't you go giving yourself airs about it.)

itowlson · 2023-02-20T02:48:31Z

Found a regression where spin up <file> (note without the -f) now gives trigger options help instead of Cannot read manifest file from "/home/ivan/spinbin/spin.toml".

itowlson · 2023-02-20T03:06:55Z

At least, a regression in the sense of "both are bad but it is a different bad." Displaying help text is arguably less surprising than firing up the app in the current directory when you thought you'd asked for a different one!

But now we're back to the positional trigger args problem: there's no definite way to distinguish spin up foo (wrong, meant to pass -f) from spin up foo (right, passing foo to the trigger)...

itowlson · 2023-02-20T03:13:00Z

To clarify, this only happens if ./spin.toml doesn't exist. If It does exist, the current behaviour is to print:

error: Found argument 'foo' which wasn't expected, or isn't valid in this context

USAGE:
    spin trigger http [OPTIONS]

For more information try --help

and this PR does not change that WHEEEE

itowlson · 2023-02-20T19:46:47Z

I piled on another heuristic to handle the spin up foo case:

$ spin up examples/http-rust
Error: Default file 'spin.toml' found. Did you mean `spin up -f examples/http-rust`?`

$ spin up --listen 127.0.0.1:4000
Serving http://127.0.0.1:4000

With this sort of hack we could go back to the fully flagless design, though at least in this one the hack is controlling only the failure mode rather than the interpretation of the application source.

radu-matei · 2023-02-22T10:13:41Z

I'm passing the -f and --from-registry flags to the same registry app:

❯ spin up --listen 127.0.0.1:3003  --from-registry ghcr.io/radu-matei/hello-registries:v1
2023-02-22T10:11:09.247402Z TRACE spin_cli::commands::up: Running trigger executor: "/Users/radu/projects/src/github.com/fermyon/spin/target/release/spin" "trigger" "http" "--listen" "127.0.0.1:3003" "--from-registry"
2023-02-22T10:11:09.272781Z DEBUG spin_loader::oci::cache: using cache root directory /Users/radu/Library/Caches/spin/registry
2023-02-22T10:11:09.289584Z TRACE spin_http: Constructed router for application hello-registries: {Wildcard(""): "hello-registries"}
Serving http://127.0.0.1:3003
2023-02-22T10:11:09.289679Z  INFO spin_http: Serving http://127.0.0.1:3003
Available Routes:
  hello-registries: http://127.0.0.1:3003 (wildcard)


❯ spin up --listen 127.0.0.1:3003  -f ghcr.io/radu-matei/hello-registries:v1
2023-02-22T10:11:51.056584Z DEBUG spin_loader::oci::cache: using cache root directory /Users/radu/Library/Caches/spin/registry
2023-02-22T10:11:52.482909Z TRACE spin_cli::commands::up: Running trigger executor: "/Users/radu/projects/src/github.com/fermyon/spin/target/release/spin" "trigger" "http" "--listen" "127.0.0.1:3003"
Error: loader error: LockedComponentSource missing source field

Caused by:
    LockedComponentSource missing source field
Error: exit status: 1

src/commands/up.rs

itowlson · 2023-02-22T18:56:10Z

Yikes, thanks to both of you for the catch!

It would be super duper useful if we had a publicly accessible demo that could be used for testing OCI bits. Or is there one in the e2e tests somewhere?

itowlson · 2023-02-22T20:00:12Z

I think this is now fixed:

# Inference
$ spin up -f localhost:5000/itowlson/http-rust:1 --insecure
Serving http://127.0.0.1:3000
Available Routes:
  hello: http://127.0.0.1:3000/hello
    A simple component that returns hello.

$ spin up -f ./examples/http-rust
Serving http://127.0.0.1:3000
Available Routes:
  hello: http://127.0.0.1:3000/hello
    A simple component that returns hello.

$ spin up -f ./examples/http-rust/spin.toml 
Serving http://127.0.0.1:3000
Available Routes:
  hello: http://127.0.0.1:3000/hello
    A simple component that returns hello.

# Forcing flags
$ spin up --from-registry localhost:5000/itowlson/http-rust:1 --insecure
Serving http://127.0.0.1:3000
Available Routes:
  hello: http://127.0.0.1:3000/hello
    A simple component that returns hello.

$ spin up --bindle spin-hello-world/1.0.0 --bindle-server http://localhost:8080/v1 --insecure
Serving http://127.0.0.1:3000
Available Routes:
  hello: http://127.0.0.1:3000/hello
    A simple component that returns hello.

$ spin up --from-file ./examples/http-rust
Serving http://127.0.0.1:3000
Available Routes:
  hello: http://127.0.0.1:3000/hello
    A simple component that returns hello.

(I clicked through all of them too to make sure they worked, but will save you the log spew.)

Thanks to @vdice for help getting a working registry and @fibonacci1729 for volunteering his app as tribute.

itowlson · 2023-02-22T20:01:43Z

Marking this as ready for review since people are testing it enough to imply this is the direction we want to go...?

radu-matei · 2023-02-22T20:39:35Z

ghcr.io/radu-matei/cloud-start:v1 should be public, so you could try with this one: https://github.com/users/radu-matei/packages/container/package/cloud-start

Alternatively, you could push an app to GHCR as well.

Signed-off-by: itowlson <[email protected]>

fibonacci1729

I gave this another round of testing this morning and the code looks reasonable to me! LGTM.

dicej

Thanks for doing this (and saving the rest of us from having to)!

dicej · 2023-02-23T19:41:17Z

src/commands/up.rs

+            (None, Some(file), None, None) => Self::infer_file_source(file.to_owned()),
+            (None, None, Some(id), None) => AppSource::Bindle(id.to_owned()),
+            (None, None, None, Some(reference)) => AppSource::OciRegistry(reference.to_owned()),
+            (_, _, _, _) => AppSource::unresolvable("More than one application was specified"),


Entirely optional slight simplification:

Suggested change

(_, _, _, _) => AppSource::unresolvable("More than one application was specified"),

_ => AppSource::unresolvable("More than one application was specified"),

Obvious in retrospect but TIL - thank you.

Signed-off-by: itowlson <[email protected]>

kate-goldenring · 2023-02-23T20:04:30Z

Haven't looked over the implementation yet but the ux was great. Worked with oci images, local files and folders, and with bindle (./target/debug/spin up --from-bindle spin-hello-world/1.0.0 --bindle-server http://127.0.0.1:8081/v1)!

itowlson requested review from fibonacci1729, kate-goldenring, lann and radu-matei February 19, 2023 23:43

itowlson mentioned this pull request Feb 19, 2023

Flagless spin up application source #1159

Closed

itowlson force-pushed the MOSTLY-flagless-app-source branch from c989fab to fc0b828 Compare February 19, 2023 23:56

fibonacci1729 reviewed Feb 22, 2023

View reviewed changes

src/commands/up.rs Outdated Show resolved Hide resolved

itowlson force-pushed the MOSTLY-flagless-app-source branch from 8789b97 to 0e1d43d Compare February 22, 2023 19:53

itowlson marked this pull request as ready for review February 22, 2023 20:01

Flagless app sources

0e34fa7

Signed-off-by: itowlson <[email protected]>

itowlson force-pushed the MOSTLY-flagless-app-source branch from 0e1d43d to 1a49cbc Compare February 22, 2023 21:06

fibonacci1729 approved these changes Feb 23, 2023

View reviewed changes

dicej approved these changes Feb 23, 2023

View reviewed changes

Better behaviour if user forgets -f flag

4153d74

Signed-off-by: itowlson <[email protected]>

itowlson force-pushed the MOSTLY-flagless-app-source branch from 1a49cbc to 4153d74 Compare February 23, 2023 19:59

itowlson merged commit 599e401 into spinframework:main Feb 23, 2023

	(_, _, _, _) => AppSource::unresolvable("More than one application was specified"),
	_ => AppSource::unresolvable("More than one application was specified"),

Conversation

itowlson commented Feb 19, 2023

Uh oh!

itowlson commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itowlson commented Feb 20, 2023

Uh oh!

itowlson commented Feb 20, 2023

Uh oh!

itowlson commented Feb 20, 2023

Uh oh!

itowlson commented Feb 20, 2023

Uh oh!

radu-matei commented Feb 22, 2023

Uh oh!

Uh oh!

itowlson commented Feb 22, 2023

Uh oh!

itowlson commented Feb 22, 2023

Uh oh!

itowlson commented Feb 22, 2023

Uh oh!

radu-matei commented Feb 22, 2023

Uh oh!

fibonacci1729 left a comment

Choose a reason for hiding this comment

Uh oh!

dicej left a comment

Choose a reason for hiding this comment

Uh oh!

dicej Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

itowlson Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

kate-goldenring commented Feb 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

itowlson commented Feb 20, 2023 •

edited

Loading