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

Support persisting Fly runtime config in a workspace secret #2714

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

jonatanklosko
Copy link
Member

flyrtconfigsecret.mp4

Copy link

github-actions bot commented Jul 18, 2024

Uffizzi Preview deployment-54362 was deleted.

@josevalim
Copy link
Contributor

Quick question: What do you think about having the buttons on the bottom, on the same row as Connect but left side? And then, when you click save, you expand the contents below it, instead of swapping the whole page? Another option is to have a "Save and connect" button, that takes you to the save screen afterwards.

Plus, in the save screen, we should make it clear on which workspace we are saving it on. :)

cpu_kind: "shared",
cpus: 1,
memory_gb: 1,
gpu_kind: nil,
gpus: nil,
docker_tag: Livebook.Config.docker_images() |> hd() |> Map.fetch!(:tag)
docker_tag: hd(docker_tags)
}
Copy link
Contributor

@josevalim josevalim Jul 18, 2024

Choose a reason for hiding this comment

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

I wonder if we should create an actual changeset for it now? It will also help us unit test the serialization and deseriaization logic.

The unit tests would help us simplify the current integration test.

Copy link
Member Author

Choose a reason for hiding this comment

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

A single changeset is tricky, in particular with volumes. We know the valid volume options only once the app loads. More importantly, in the volume section we have a "Create volume" form, and since we can't have nested forms, it means we can't really have a single form for all the config fields.

socket
|> assign(
token: config_defaults["token"],
app_name: config_defaults["app_name"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Curiosity: why token and app name are not part of specs, so they are all handled in a single place? I guess they have separate loading workflows?

Copy link
Member Author

@jonatanklosko jonatanklosko Jul 19, 2024

Choose a reason for hiding this comment

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

Yeah, they have a more central role for loading, conditionals, we use them for requests, so handling them individually feels cleaner. Also, when they are empty, instead of showing form errors, we show relevant message boxes with instructions.

Initially I was going for a single changeset, but I ended up with this approach, and with the nested volumes form we can't really include everything in the changeset anyway.

@jonatanklosko
Copy link
Member Author

Quick question: What do you think about having the buttons on the bottom, on the same row as Connect but left side? And then, when you click save, you expand the contents below it, instead of swapping the whole page? Another option is to have a "Save and connect" button, that takes you to the save screen afterwards.

I tried different versions and I'm not really sure. I feel that having buttons at the bottom is a bit weird because the idea is that the form is "expanding" as you enter the token and app, and the fixed buttons break that. Another option would be to have the load button as is, and show save at the bottom only once there is connect, but then they are no longer grouped. If you have a stronger preference let me know. As for "Save and connect", I would not couple these two actions.

Plus, in the save screen, we should make it clear on which workspace we are saving it on. :)

Good call, done!

@jonatanklosko jonatanklosko merged commit 98169e8 into main Jul 19, 2024
7 checks passed
@jonatanklosko jonatanklosko deleted the jk-fly-runtime-persistence branch July 19, 2024 08:29
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