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

Non interactive databases take two #151

Merged
merged 5 commits into from
Nov 14, 2023

Conversation

itowlson
Copy link
Contributor

@itowlson itowlson commented Nov 8, 2023

This is another take on #125 (cf. #126). It's significantly more complicated than #126, but aims to be more explicit.

I am not delighted with the CLI experience in this initial draft. This looks like:

spin cloud deploy --link sqlite:default=* --link sqlite:monies=finance

Here the sqlite: prefix says what the label refers to (to allow for K/V or other resource linking in future), and * is a magic symbol for "create a new and link that". This is a lot more verbose than @kate-goldenring's example syntax in #126 (comment) and perhaps the future-proofing is too speculative, I'm not sure; and of course magic symbols are never great for discoverability.

That said, I'm optimistic about the principle of using a strategy object, and I think I've separated the CLI parsing reasonably cleanly from the strategy object itself, so maybe we can iterate on this and find an experience that folks are happy with?

(Oh yeah there are a whole lot more tests need writing but the mocks had turned my brain to sausage so I'll do them later.)

@kate-goldenring
Copy link
Collaborator

kate-goldenring commented Nov 9, 2023

@itowlson i haven't looked into the implementation yet but I am a little confused on what * means here:spin cloud deploy --link sqlite:default=*. How do we know which DB to link it to?

You're amazing inline docs clear this up:

    /// do not exist will be created. To express "create a new database with
    /// a random name", use the special database '*'.

@itowlson
Copy link
Contributor Author

itowlson commented Nov 9, 2023

chuickle but this does identify the non-obviousness of the syntax, especially since * means something different if it precedes the = sign.

I did wonder about using magic expressions such as sqlite:default=new(). I am not sure if that is better or worse. Or what the syntax for "for databases not explicitly listed..." would look like.

/// do not exist will be created. To express "create a new database with
/// a random name", use the special database '*'. To link all labels
/// not specified in other --link flags, use the special label '*'.
/// Thus, 'sqlite:*=*' would create a new randomly named database for
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did wonder about using magic expressions such as sqlite:default=new(). I am not sure if that is better or worse. Or what the syntax for "for databases not explicitly listed..." would look like.

One thing i wonder is whether we should move the wildcard cases * to a separate PR while we talk it out and for now go with the requirement of specifying database and label names. I think the * is fairly sound but in *=, the * acts differently than =* namely, one means "any (label) that exists" and the other means "create a new db for each". Overall, the label wildcard makes more sense to me as being desired in non-interactive scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kate-goldenring That's a wise call. Thanks!

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson itowlson force-pushed the non-interactive-databases-take-two branch from 5998861 to 9d0b8c5 Compare November 14, 2023 01:56
@itowlson itowlson marked this pull request as ready for review November 14, 2023 01:57
@itowlson
Copy link
Contributor Author

@kate-goldenring updated

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Copy link
Collaborator

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thank you @itowlson!

@itowlson itowlson merged commit 83e6a84 into fermyon:main Nov 14, 2023
8 checks passed
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.

None yet

2 participants