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

Update SQLite subcommand to use links #75

Merged
merged 4 commits into from
Sep 29, 2023

Conversation

kate-goldenring
Copy link
Collaborator

@kate-goldenring kate-goldenring commented Sep 1, 2023

PR for managing databases and links.

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looking great!

storage_id: &str,
create_default_database: bool,
) -> Result<Uuid> {
pub async fn add_app(&self, name: &str, storage_id: &str) -> Result<Uuid> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was already called add_app before, but I do wonder why we didn't call it create_app? There are other places where we might want to add an app to something that this might become even more confusing.

crates/cloud/src/client.rs Show resolved Hide resolved
crates/cloud/src/client.rs Outdated Show resolved Hide resolved
crates/cloud/src/client.rs Outdated Show resolved Hide resolved
crates/cloud/src/client.rs Outdated Show resolved Hide resolved
src/commands/link.rs Outdated Show resolved Hide resolved
src/commands/link.rs Outdated Show resolved Hide resolved
src/commands/link.rs Outdated Show resolved Hide resolved
src/commands/link.rs Outdated Show resolved Hide resolved
src/commands/sqlite.rs Outdated Show resolved Hide resolved
@rylev
Copy link
Contributor

rylev commented Sep 28, 2023

I'm going to mark this as ready to review, because it works well already.

@rylev rylev marked this pull request as ready for review September 28, 2023 09:04
Create links subcommand
Use new upstream APIs for linking apps to databases

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
Co-authored-by: Ryan Levick <ryan.levick@fermyon.com>
@kate-goldenring
Copy link
Collaborator Author

@rylev this is a lot of commits but rebasing was a headache. If you aren't opposed, i copied all changes to a different branch. I could push that branch into this one to get us down to one commit: https://github.com/fermyon/cloud-plugin/compare/main...kate-goldenring:sql-links-cli?expand=1

@rylev
Copy link
Contributor

rylev commented Sep 28, 2023

That might be easiest... I wouldn't be opposed.

@kate-goldenring
Copy link
Collaborator Author

@rylev done 🎉

@kate-goldenring kate-goldenring marked this pull request as draft September 28, 2023 21:30
@kate-goldenring
Copy link
Collaborator Author

kate-goldenring commented Sep 28, 2023

@rylev i think we should figure out the following before merging:

  1. What is the default DB name
    2. We should improve the deploy flow for when a user cancels db selection during a deploy. It breaks future attempts because the oci image is pushed and storage id is set: fixed
  2. CLI experience for list -- or we can keep iterating on this

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
@kate-goldenring kate-goldenring marked this pull request as ready for review September 29, 2023 02:57
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev
Copy link
Contributor

rylev commented Sep 29, 2023

Talking it over with others, we've decided to go with random name generation for now. We can always revisit this in the future.

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev merged commit fd5031c into fermyon:main Sep 29, 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