My first CLI tool in Rust for migrating repositories from Bitbucket to GitHub

[arkus]

Hi all!

I'm fairly new to Rust (coming from the JS/TS world with a few years of mobile dev experience, both Android and iOS) and I'm trying to hack some utility things to learn more about the language and how to write idiomatic Rust.

I want to share with you my first "real" CLI I've built that me and my teammates used at work to migrate a bunch of repositories from our Bitbucket to a new GitHub organization. Moving 300+ repositories manually would be a real pain in the ass, so I naturally thought about automating this. The first option was hacking some bash scripts or using zx for scripts in TS but then an idea came to my mind that I could use Rust for it, as I've heard it's a great project idea for beginners to create CLI that solves their real-life problems

That's how migrate-bb-to-gh was born.

This is not the latest creation (dating April/May 2022) but I finally got over myself and decided to share it here and ask for a code review.

Some more info on how it works

It lets you choose (via interactive prompts using dialoguer) which repositories you want to migrate from the Bitbucket organization to the GitHub organization with some additional settings like what should be the default branch, which Team should get access to this new repository (if you don't have a proper Team set up, you can set up it via CLI and add members to the team as well), assign proper access to the repository for selected teams, etc.

The migration is split into 2 steps:

1. You select what you want to migrate and how, this stores all migration actions in a JSON file that you can double-check, edit, and share with someone on the team
2. Run the actual migration based on the file with migration actions

I would really appreciate it if you had some time to check the code and let me know your opinion of it. Unfortunately, I don't have any colleagues working in Rust so couldn't think of another place to ask for this.

Link to the repo: https://github.com/arkus7/migrate-bb-to-gh

[afetisov]

That's a very nice project! It's clear that a lot of work went into polishing it. The code is very readable, nicely structured, pretty, and follows the best practices. But of course, there's always something to improve :)

---

You say in the readme

The configuration file contents are "encrypted" before they are put inside binary

and you write a file config.encrypted.yml, but the contents are just base64 encoding of the config. This is not encryption. Not even a poor man's encryption. I assume you know that, since you put "encrypted" in scare quotes, but regardless, such description is dangerously misleading. It may make the user assume that the configs are somehow secure, when they are not. If you are encoding in base64, just say so explicitly.

Even an encryption with a static key hardcoded into the binary would be better. At least the attacker would have to know both the config and the binary, and have either the source code or the skills to extract the key. base64 can be decoded by literally anyone, and it's quite easy to spot.

---

let migration: Migration = serde_json::from_reader(file).with_context(|| format!("Error when parsing {} file.\nIs this a JSON file?\nDoes the version match the program version ({})?\nConsider re-generating the migration file with `wizard` subcommand.", &self.migration_file.display(), &self.version))?;

You can format this huge line in a more readable way using multiline string literals:

let migration: Migration = serde_json::from_reader(file)
    .with_context(|| format!(
        "Error when parsing {} file.\n\
        Is this a JSON file?\n\
        Does the version match the program version ({})?\n\
        Consider re-generating the migration file with `wizard` subcommand.",
        &self.migration_file.display(),
        &self.version,
    ))?;

Note that you must still explicitly specify \n at the end of the line, since the end-of-line \ consumes all following whitespace.

---

if !confirmed {
    return Err(anyhow!("Migration canceled"));
}

You can use anyhow::ensure! for that:

ensure!(confirmed, "Migration canceled");

---

It may be beneficial to use the .context() method from anyhow more often. Currently the methods just bubble up the root error up the stack, which is usually not very enlightening. Low-level methods also often produce very poor errors. By using .context() you can basically recreate pretty artisanal stacktraces, with extra required context such as file paths.

By the way, did you know that anyhow can automatically capture stack traces? You need to enable the backtrace feature, and set the corresponding environment variable during execution.

---

There are also a bit too many hardcoded paths for my liking. I mean the paths and arguments of external executables, and the URLs of Github API.

---

Again, that's a very well-written project! You have dived right in, and handled beautifully all the API and async stuff. I don't think I could write it any better.

[arkus]

Thank you for kind words, I really appreciate it!

As you mentioned, I did spent some time to polish the code as first version had a lot of rough edges.

When it comes to "encrypted" config - thanks for notice that this might be misleading. I did put in the Readme information that this is not secure. I didnt think of encrypting it with a static key at the time. I did it mainly because otherwise I was including whole config.yml file in the binary and it was really easy to extract the tokens, ssh keys etc from the binary. So the first, easiest thing that came to my mind was simple base64.

I didn't know about the ensure method of anyhow, I'll definitely use it!

I was thinking about replacing some String struct members with &str, but I didn't want to deal with lifetimes at that time. Do you see anything that should be definitely done using lifetimes instead of owned values?

[afetisov]

Nothing which jumped on me. In general dealing with lifetimes on structs is a huge pain and is best avoided, because a lifetime-bound struct cannot be returned from the function (unless you borrow from one of the arguments). You certainly don't need zero-cost (de-)serialization for the case of a migration api client.