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

WIP: support a globally-installable escript #232

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

gworkman
Copy link

tl;dr

I added support to install NervesHubCLI as an escript, to evaluate whether this provides a better experience for working with NervesHub from the command line without configuring it with every project. It's quite rough around the edges

Problem

I recently was invited to try out the NervesCloud beta by Lars and Josh. This was my first time setting up NervesHub, and unfortunately I found the process to be confusing. While NervesHubLink makes sense to add as a project dependency, I couldn't quite figure out how to work with the mix tasks for the CLI.

I know that 2.0 is under active development and the docs are getting better with every commit, and that definitely would have helped with some of the issues I faced. However, one of the most confusing parts of this was configuring the CLI to correctly talk with the NervesHub instance (NervesCloud in this case), and then realizing that I need to run mix nerves_hub.user auth (previously there were no error messages, just a crash that the %NervesHubCLI.API.Auth{} struct was nil). I did get it working eventually, but while talking to Josh and Lars, I realized that NervesHubCLI may have a better user experience if it was a globally-installable CLI tool, with some persistent settings.

Potential solutions

There are a number of different ways which this problem could be tackled.

  • A hex archive - similar to the phx_new generators, or nerves_bootstrap. Installation could be as simple as mix archive.install hex nerves_hub_cli
  • #! /usr/bin/env elixir script, with some Mix.install commands.
  • Modifying the project to support installation as an escript

For the first option of distributing it as a hex archive, I think this makes the most sense, and would be the easiest. However, hex archives are supposed to be lightweight extensions of Mix, so unfortunately, it can't have dependencies to prevent conflicts with the project deps. Given that the CLI has web requests and cryptography functions, I don't think it's a good idea to try to go without any dependencies

I made some preliminary experiments with the Elixir script idea, but I found that the Mix module was not available in the environment, which meant that I couldn't easily reuse the current mix tasks found in this repo, as they make heavy use of that module.

I think escript makes the most sense, as it can be easily distributed and doesn't require a large number of changes.

Changes

To build the dependency as an escript, I basically copied all of the mix tasks into the NervesHubCLI.CLI module. I removed references to Mix, and swapped out the task-specific modules used (such as lib/mix/nerves_hub/*.ex) into variants which didn't have Mix dependencies. For example, instead of using Mix.Shell.yes?/1, I created a variant which just reads stdio via IO.gets/1 and checks to see if it is a positive response.

The command structure is now slightly different. Because it is no longer invoked with mix tasks, the command follows a command + subcommand structure. Examples:

  • $ nerves_hub user whoami
  • $ nerves_hub device list
  • $ nerves_hub key create --name dev_key

Additionally, there are some persistent config options which can now be set. I anticipate support for these can be expanded in the future, but the main one is the uri option:

  • $ nerves_hub config set uri "https://manage.nervescloud.com"
  • $ nerves_hub config get uri

What's broken

This PR is definitely not ready. I wanted to showcase this early work, to see if this CLI experience is something which would be a beneficial change for the community. As such, I spent time to get some basic functions to work, but didn't spend a lot of time to polish. Some things which are definitely broken:

  • The :ca_store dependency stores the cert files in the priv/ directory - which is not available for escripts. So I commented out portions of the HTTP API interface which used that.
  • There are still some places where Mix is used. For example, try searching for Mix.Project.config() in the lib/nerves_hub_cli/cli/ folder. If you run a command which calls Mix it will crash
  • I didn't update the error messages/help docs. Error messages will still refer to the mix tasks
  • Testing is non existent at this point (though 99% of the changes are coping code from the mix tasks into their own modules)
  • You may need to specify the flags for org or product (or set them as environment variables: NERVES_HUB_ORG and NERVES_HUB_PRODUCT), since they aren't read from application config.

How to test it out

I'm assuming you are using the NervesCloud instance here, if not change the uri to point to your own instance.

mix escript.install git https://github.com/gworkman/nerves_hub_cli branch gw-make-escript

# Note: you may need to add the escript directory to your path. Follow the instructions from escript

nerves_hub config set uri "https://manage.nervescloud.com/"
nerves_hub user whoami
nerves_hub user auth
nerves_hub user whoami
nerves_hub firmware list --org my_org --product my_product

# to uninstall
mix escript.uninstall nerves_hub

What's next

If this is something which is useful for the community, I'll keep working on this to polish it up. For the most part, I haven't touched anything which breaks the mix tasks (just the removed :ca_store, and added global config of the uri).

I'm happy to get some feedback, and also some input on some of my opinionated things (such as naming the executable to nerves_hub or the module names - NervesHubCLI.CLI.* isn't great imo, but I wasn't sure where else to put it).

|> File.read!()
|> X509.from_pem()
|> Enum.map(&X509.Certificate.to_der/1)
# CAStore uses the priv directory, which does not work with escripts
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an interesting issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the OS-supplied certs are returned in the true clause. It seems like this code could be completely deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

you are right, i guess we can just state that you need to keep your os supplied certs up to date

@joshk
Copy link
Contributor

joshk commented Sep 15, 2024

I love where this is going!

Regarding setting the org and product, do you have any options/ideas to reduce the need to specify these on every cli use?

@gworkman
Copy link
Author

Hey @joshk, thanks for the support! This is a really good question, but I have to start with the disclaimer that I am still quite new to NervesHub, so I haven't spent much time working with multiple orgs/products. But I did spend some time thinking about this use case, and here are several options for how I think we could support these settings:

  1. For the casual user, or small teams who might only have one org and multiple products, or even just one product: allow configuration of organization and/or product at the global level, using nerves_hub config set org my_org and nerves_hub config set product my_product. But in this case, it would be very easy for users to inadvertently use the wrong configuration when starting new projects. So the CLI should probably notify the user of the global setting each time and maybe even ask for confirmation
  2. For users who are switching between orgs and products frequently, I suggest that we write the docs to specifically mention the environment variables. That way it should be easy enough to source .env files, or use direnv or other tools to manage the project environment settings. This is actually already supported, so no changes are necessary
  3. Another option would be to support a --local flag for the nerves_hub config set org my_org command. This could work in two ways: add a nerves-hub.config file in the current project directory which is checked when any commands are run, or, we could utilize the global configuration file to store the path to the current directory as a key with the config options to apply for the local directory (which is currently an Elixir map which gets stored and loaded with :erlang.term_to_binary). For example, the global config would look like:
%{
  token: "nh_token_here",
  uri: "https://manage.nervescloud.com",
  local_config: %{
    "/path/to/my/project" => %{
      org: "my_org",
      product: "my_product"
    },
    "/path/to/other/project" => %{
      org: "my_other_org",
      product: "my_other_product"
    }
}

However, this solution breaks down when the project is moved, renamed, etc.

@joshk
Copy link
Contributor

joshk commented Sep 17, 2024

@gworkman I really love this!

I'd be happy to merge this in ASAP and for us to continue to improve it.

I think we need to address the CAStore code that is commented out. Could we wrap it in an if and only load the certs from CAStore if the CLI isn't being used via escript?

|> File.read!()
|> X509.from_pem()
|> Enum.map(&X509.Certificate.to_der/1)
# TODO: These conditions can probably be removed after successfully testing for some time
Copy link
Author

Choose a reason for hiding this comment

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

TODO: check security considerations of loading the CA certs at compile time into the binary, and the checks here

@joshk
Copy link
Contributor

joshk commented Oct 16, 2024

@gworkman can you also remove Elixir 1.11, 1.12, and 1.13 from the GitHub Actions configs?

Comment on lines +8 to +12
@castore_certs CAStore.file_path()
|> File.read!()
|> X509.from_pem()
|> Enum.map(&X509.Certificate.to_der/1)

Copy link
Contributor

Choose a reason for hiding this comment

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

@fhunleth @jjcarstens do either of you have any concerns with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok. This is a really big PR that looks like a clear improvement so I mostly want to get it in and iterate.

@gworkman
Copy link
Author

@joshk will do! I think it's about ready to merge in. There's one or two more spots I need to highlight for review

@gworkman
Copy link
Author

@joshk updated min elixir version and CI config for 1.13, in line with what Frank said on slack that the rest of Nerves is >= 1.13

@lawik lawik marked this pull request as ready for review October 18, 2024 09:31
@lawik
Copy link

lawik commented Oct 18, 2024

help doesn't seem to work. And all variants of help <cmd> or adding -h, --help in the command should ideally work. But we can do that next. This is so much smoother. Just tried it.

@lawik
Copy link

lawik commented Oct 18, 2024

For the iterations:

If we could set config like uri as config per org I think lots of people get used to doing -o community-fleet or whatever as they work with Fly's tooling that generally requires that. We could try to do what they do and when not specified try to infer the config from the current project directory. But that might get messy.

Copy link
Author

@gworkman gworkman left a comment

Choose a reason for hiding this comment

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

Ready for review & merge. Please see the two highlighted points -
nerves_hub device burn, nerves_hub firmware sign and nerves_hub firmware publish will all fail due to references to Mix module

"""
@spec firmware() :: Path.t()
def firmware do
# TODO: what to replace these paths with?
Copy link
Author

Choose a reason for hiding this comment

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

@joshk @fhunleth I don't think I understand the use of :images_path key here... I'm assuming it is a special key set by Nerves?

Basically, we need to change this because config/0 resolves to Mix.Project.config(), which we don't have access to at runtime, nor can we call Mix.Project.build_path()

So how should we get this path?

|> OptionParser.to_argv()

# TODO: remove reference to mix task
Mix.Task.run("burn", burn_args)
Copy link
Author

Choose a reason for hiding this comment

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

@joshk @fhunleth this is the last reference to Mix. I can replace it with System.cmd but that feels wrong? Not sure what the best way to otherwise call the mix task would be

@gworkman
Copy link
Author

help doesn't seem to work. And all variants of help <cmd> or adding -h, --help in the command should ideally work. But we can do that next. This is so much smoother. Just tried it.

Correct. Mix tasks had special docs so you could run mix help TASK_NAME, I didn't get around to implementing the equivalent feature yet. But it can be the next PR :)

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.

4 participants