Migrate to CLI v3 #304

nebulatgs · 2023-03-03T15:11:16Z

The repository tracking changes can be found at https://github.com/railwayapp/cliv3

Summary of Changes

Migrate to using Rust with Clap for CLI args
Use the v2 Public API
Support for railway service to link to a service
railway domain to generate a service domain
Add various flags for use in CI/CD scripts

.github/workflows/release.yml

.github/workflows/label-check.yml

ndneighbor · 2023-03-03T16:28:14Z

.github/workflows/tag.yml

@@ -1,58 +0,0 @@
-name: Tag


Slight nit: this comes from years of managing yml. I personally find .yml files unreadable whenever you have any yml file above 100 lines. There is some opportunity to maybe split the .ymls and steps, but I think this is fine for now and dependent on personal opinion.

Why do you think we should have merged the files in your opinion?

CONTRIBUTING.md

Cargo.toml

README.md

uninstall.sh

src/config.rs

coffee-cup · 2023-03-03T16:38:46Z

src/consts.rs

+pub const NO_SERVICE_LINKED: &str =
+    "No service linked and no plugins found\nRun `railway service` to link a service";
+pub const ABORTED_BY_USER: &str = "Aborted by user";
+pub const PROJECT_NOT_FOUND: &str = "Project not found!";
+pub const SERVICE_NOT_FOUND: &str = "Service not found!";
+pub const NON_INTERACTIVE_FAILURE: &str = "This command is only available in interactive mode";


Nit: Should these be a single error enum with a custom Display?

Not sure, is that more idiomatic?

I think would be a little bit. But very low priority in the grand scheme of things

src/entities.rs

src/commands/link.rs

src/commands/domain.rs

src/commands/run.rs

src/commands/starship.rs

JakeCooper

I'd say in general we need to take a pass over this to make it more clear and clean

I know you're trying to land this for today, so we can just fix the flags I've added for now, but we're gonna want to go over this again and iron it out because IMO, there are a few places for which it's a bit awkward

src/client.rs

JakeCooper · 2023-03-03T19:21:52Z

src/commands/add.rs

+    } else {
+        inquire::MultiSelect::new("Select plugins to add", filtered_plugins)
+            .with_render_config(render_config)
+            .prompt()?
+    };


Can remove the else since you have a return in the if already

When we can, please try to keep code linear

Logic trees should be avoided if early returns will do (which they usually will)

This isn't an early return, it's a conditional value (similar to a ternary expression in other languages), so the else is needed

JakeCooper · 2023-03-03T19:24:45Z

src/commands/add.rs

+        if !std::io::stdout().is_terminal() {
+            println!("Creating {}...", plugin);
+            post_graphql::<mutations::PluginCreate, _>(&client, configs.get_backboard(), vars)
+                .await?;
+        } else {
+            let spinner = indicatif::ProgressBar::new_spinner()
+                .with_style(
+                    indicatif::ProgressStyle::default_spinner()
+                        .tick_chars(TICK_STRING)
+                        .template("{spinner:.green} {msg}")?,
+                )
+                .with_message(format!("Creating {plugin}..."));
+            spinner.enable_steady_tick(Duration::from_millis(100));
+            post_graphql::<mutations::PluginCreate, _>(&client, configs.get_backboard(), vars)
+                .await?;
+            spinner.finish_with_message(format!("Created {plugin}"));
+        }
+    }


Is there no library which supports optional spinners for non tty environments?

My comment was going to be we can probably pull this into some sort of textual CLI library but there's gotta be one that supports both interactive as well as non interactive terminals

Or just abstract it ourselves. We shouldn't have to duplicate the gql query every time depending on if we are in a terminal environment or not

JakeCooper · 2023-03-03T19:27:45Z

src/commands/domain.rs

+    let service = if body.project.services.edges.len() == 1 {
+        body.project.services.edges[0].node.clone().id
+    } else {


Does line 35 return? If so, we should be using explicit returns

Implicit returns are fine for small functions like map/filter/return but should be avoided, for clarity, here

Also, if the else is going to bail, we should flip the branches and have an early return to remove the else case

This another instance of a conditional value

JakeCooper · 2023-03-03T19:28:59Z

src/commands/domain.rs

+    if !std::io::stdout().is_terminal() {
+        println!("Creating domain...");
+
+        let res = post_graphql::<mutations::ServiceDomainCreate, _>(
+            &client,
+            configs.get_backboard(),
+            vars,
+        )
+        .await?;
+
+        let body = res.data.context("Failed to create service domain.")?;
+        let domain = body.service_domain_create.domain;
+
+        println!("Service Domain created: {}", domain.bold());
+    } else {
+        let spinner = indicatif::ProgressBar::new_spinner()
+            .with_style(
+                indicatif::ProgressStyle::default_spinner()
+                    .tick_chars(TICK_STRING)
+                    .template("{spinner:.green} {msg}")?,
+            )
+            .with_message("Creating domain...");
+        spinner.enable_steady_tick(Duration::from_millis(100));
+
+        let res = post_graphql::<mutations::ServiceDomainCreate, _>(
+            &client,
+            configs.get_backboard(),
+            vars,
+        )
+        .await?;
+
+        let body = res.data.context("Failed to create service domain.")?;
+        let domain = body.service_domain_create.domain;
+
+        spinner.finish_and_clear();
+
+        println!("Service Domain created: {}", domain.bold());
+    }


Many cases here of duplicate logic related to the is_terminal() call with spinner and non spinner

I'd suggest either finding a CLI output library which supports this natively for both tty and non-tty, or compartmentalizing your logic into a library

JakeCooper · 2023-03-03T19:31:39Z

src/commands/init.rs

+    let name = inquire::Text::new("Project Name")
+        .with_formatter(&|s| {
+            if s.is_empty() {
+                "Will be randomly generated".to_string()
+            } else {
+                s.to_string()
+            }
+        })
+        .with_placeholder("my-first-project")
+        .with_help_message("Leave blank to generate a random name")
+        .with_render_config(render_config)
+        .prompt()?;
+
+    let name = if name.is_empty() {
+        use names::Generator;
+        let mut generator = Generator::default();
+        generator.next().context("Failed to generate name")?
+    } else {
+        name
+    };


This logic is kinda brutal to follow

Suggestion: replace it with a "prompt or generate" function which:

Prompts

Returns if value

returns a generated name

IMO much cleaner logical flow than the "Prompt, check, hoist-in-else" behavior. That last part I really think we should avoid. I'm still only 99% certain that's what it's doing

Agreed. In general I think we should abstract out the inquire library so we can do things like

let name = prompt("Project Name", "{placeholder}", "{help message}").or_else(|| random_name())

or some variation of that. Basically condensing these lines

I think this is a great idea, will do!

JakeCooper · 2023-03-03T19:33:01Z

src/commands/link.rs

+        let environment = if let Some(environment_name_or_id) = args.environment {
+            let environment = body
+                .project
+                .environments
+                .edges
+                .iter()
+                .find(|env| {
+                    env.node.name == environment_name_or_id || env.node.id == environment_name_or_id
+                })
+                .context("Environment not found")?;
+            ProjectEnvironment(&environment.node)
+        } else if !std::io::stdout().is_terminal() {
+            bail!("Environment must be provided when not running in a terminal");
+        } else {
+            inquire::Select::new(
+                "Select an environment",
+                body.project
+                    .environments
+                    .edges
+                    .iter()
+                    .map(|env| ProjectEnvironment(&env.node))
+                    .collect(),
+            )
+            .with_render_config(configs.get_render_config())
+            .prompt()?
+        };


Logic that probably would benefit from being pulled into a function

JakeCooper · 2023-03-03T19:35:32Z

src/commands/unlink.rs

+    let confirmed = !if std::io::stdout().is_terminal() {
+        inquire::Confirm::new("Are you sure you want to unlink this project?")
+            .with_render_config(configs.get_render_config())
+            .with_default(true)
+            .prompt()?
+    } else {
+        true
+    };


else val type function here.

Generally, if you're using else, the code can be cleaned up

ndneighbor

Green, expected fast follow on for code quality.

nebulatgs added 3 commits March 3, 2023 10:02

delete everything

e7a7fe1

migrate to v3

2f0ad75

basic logout command

87eb43a

nebulatgs added the release/major Author major release label Mar 3, 2023