Rust rewrite

[nellshamrell]

Begins/continues Rust rewrite using the Serenity library.

The README file has lots of details about how it works!

Note - Although Serenity does employ the concept of owners for bots, I have chosen to manually implement an authorized users list which is populated via an env variable. The reason is that we may want finer grained controls over who can do what - for example an authorized user can run a certain subset of commands, an admin user can run all commands. I am definitely willing to revisit this in the future as Serenity continues to be developed.

[JohnTitor]

nit: sudo is meaningless, can be removed.

[nellshamrell]

Fixed!

[smarnach]

Does the bot only listen to direct messages sent to it, or can it be configured to listen on a particular channel?

[smarnach]

We are in the process of migrating the crates.io repo to GitHub Actions instead of Travis. We probably want to use GitHub Actions here as well in the long run.

[nellshamrell]

Of course! Happy to do that in the long run.

[nellshamrell]

@smarnach - haven't gotten to that point in the design yet - it currently listens to all channels on a Discord server for commands, but I can definitely configure it to only listen on a specific channel if that is desired. Figured I would leave that decision until more of the design is complete.

[nellshamrell]

Added an issue to migrate from Travis CI to GitHub actions here #14. I'd rather address that in a separate PR and I am happy to address that next after this is merged :)

[XAMPPRocky]

@nellshamrell We actually just merged the simpleinfra/simple-ci github action. Which should make it easy to test. Here's a complete CI config for GitHub Actions.

.github/workflows/main.yml

name: Rust
on: [push, pull_request]
env:
  CARGO_TARGET_DIR: target
  AUTHORIZED_USERS: "luke,leia"
jobs:
  build:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
    # Set rustup toolchain
    - run: rustup default ${{ matrix.channel }}
    - uses: rust-lang/simpleinfra/github-actions/simple-ci@master
    strategy:
      matrix:
        channel: [stable, beta, nightly]

[XAMPPRocky]

@nellshamrell Sorry I posted maybe too quickly 😅 I updated it to match what the current travis had.

[nellshamrell]

Added in the.github/workflows/main.yml file but it looks like I can't trigger it just yet. I suspect this may be because only collaborators on the repo can add workflows. @sgrif or @pietroalbini could you add me as a collaborator on this project?

[XAMPPRocky]

@nellshamrell It won't appear here because it has to be merged. You can see the workflow running on your fork. It's all green :) https://github.com/nellshamrell/crates-io-ops-bot/actions

[nellshamrell]

@XAMPPRocky Yay! Thank you!

Clearly the long weekend cobwebs have not cleared from my mind yet!

[pietroalbini]

Thanks for this Nell! ❤️

Left a few comments, mostly related to authentication.

[pietroalbini]

Hmm, can we avoid relying on .env for tests? It'd be weird to see test failures just because .env changed.

[XAMPPRocky]

Well it's not actually dependent on the .env for tests. On CI it's set through the GitHub workflow, the .env is only for local dev.

[pietroalbini]

I'd still prefer if the tests were self-contained and they didn't depend on an environment variable set outside of the process.

[nellshamrell]

I can understand the desire, but it feels like we're going against the intent of dotenv to set the local testing environmental variable within the test rather than within the .env file.

[nellshamrell]

I have clarified the comment a bit more

[nellshamrell]

And I'm willing to revisit this in the future if it proves confusing

[nellshamrell]

Actually...after breaking my CI tests by forgetting to update the env variable, I think you're right on this one. Will change that today :)

[smarnach]

I also feel fuzzy about unit tests depending on environment variables in this particular way. It's pretty normal to depend on the environment for e.g. database auth in tests, but this depends on particular values that are only provided in the CI configuration.

If the configuration gets more complex, I'd prefer to split the code initializing the configuration from the code consuming the configuration. Roughly something like this (completely untested):

struct Config {
    authorized_users: Vec<String>,
}

impl Config {
    fn new(authorized_users_str: &str) -> Self {
        let authorized_users = authorized_users_str.split(',').map(String::from).collect();
        Config { authorized_users }
    }

    fn is_authorized(id: &str) -> bool {
        self.authorized_users.contains(id)
    }
}

impl Default for Config {
    fn default() -> Self {
        Config::new(
            &dotenv::var("AUTHORIZED_USERS").expect("env variable AUTHORIZED_USERS must be set"),
        )
    }
}

This way, everything except for the Default implementation can be tested independently from the environment, and the default implementation doesn't really need testing, since it just plugs strings from the environment into the Config::new() function.

(If Config gets more complex, new() should be replaced by a builder.)

But yeah, getting something that works first may be a more important goal at this stage. :)

[smarnach]

This should probably take an &str – there is no reason why checking whether a user is authorized should consume the string.