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

Improve CLI help, add versioning, & list/retrieve enhancements #30

Merged
merged 12 commits into from
Dec 19, 2023

Conversation

dtbuchholz
Copy link
Contributor

@dtbuchholz dtbuchholz commented Dec 18, 2023

Summary

The primary motivation was are around adding new CLI help documentation for all commands, including adding flag aliases any shared flags across multiple commands. And for every command, the usage, description, and options documentation has been enhanced to show usage patterns.

There are also a few of new features:

  • Rename the wallet command to account, and its pubkey subcommand to address—for consistency purposes.
  • Add a version global flag.
  • Add flag for list output to be formatted as a json array (vs. a text block of all vault names).
  • Add a flag for --output on the retrieve command for writing to the current dir (existing logic), custom dir (new feature), or stdout (new feature, useful with car extract piping).

Part of the motivation to keep things as-is with required flags vs. moving to positionals is because we could use EnvVars with those flags. This isn't implemented but would make it easier to pass the account, vault, etc. via an env var. And on top of that, maybe even include those in the config.yaml file as another way to automatically give the commands context.

Details

wallet renamed to account

I only did this because all of the other commands seem to reference passing the account / the account's address. So, an account command feels a little bit more clear than a wallet command. But, this change can easily be reverted back to wallet/pubkey, so I'm open to feedback!

Versioning

For every push on main, a GH action will check the latest tag/release and write to a version.json file, which is read when the vaults command is installed. Here's an example of how it works. Thus, when you run vaults -V, it'll log v0.0.6.

List

The list command can now take a --format json flag, letting you log the results as a json array. This basically lets you get the same response as https://basin.tableland.xyz/vaults?account=xxx.

> vaults list --account 0xf78A936dBeFFcc4619214296eA559Cf0C35BC642 --format json
["wxm_demo.wxm_geo","wxm_demo.wxm_geodata","wxm_demo.my_table","xm_test.my_table","wxm_demo.push_parquet","xm_test.cache_parquet","test.new_vault","test.new_vault_2","test.new_vault_3"]

Retrieve

The default functionality will download a CAR file to the current working directory; no changes to the logic. But, you can now also pass a defined directory path with the --output flag and download the file there—and the --output flag has an alias of -o. Similar usage pattern as lassie.

vaults retrieve -o ./test bafybeigpvzdvjgde5a6ayj7265a6ns3i4mvhhr6pqtlebpil5imkeh4ncy

Or, you can pipe its contents to stdout. Under the hood, this downloads the CAR file as a temp file and then streams the contents to stdout out, before cleaning up the file.

vaults retrieve -o - bafybeigpvzdvjgde5a6ayj7265a6ns3i4mvhhr6pqtlebpil5imkeh4ncy | car extract

Help output

Here's an example of what shows when you run vaults create --help. Namely, it more explicitly defines the "Usage" field, adds a "Description" field with an example, and shows which flags are purely optional vs required for the command to succeed. Many of the command also have new aliases

NAME:
   vaults create - Create a new vault

USAGE:
   vaults create [command options] <vault_name>

DESCRIPTION:
   Create a vault for a given account's address as either database streaming or file uploading. Optionally, also set a cache duration for the data.

   EXAMPLE:

   vaults create --account 0x1234abcd --cache 10 my.vault

OPTIONS:
   OPTIONAL:

   --cache value               Time duration (in minutes) that the data will be available in the cache (default: 0)
   --dburi value               PostgreSQL connection string (e.g., postgresql://postgres:[PASSWORD]@[HOST]:[PORT]/postgres)
   --provider value, -p value  The provider's address and port (e.g., localhost:8080) (default: https://basin.tableland.xyz)
   --window-size value         Number of seconds for which WAL updates are buffered before being sent to the provider (default: 3600)

   REQUIRED:

   --account value, -a value  Ethereum wallet address

@dtbuchholz
Copy link
Contributor Author

@brunocalza i was hacking around over the weekend and made some changes that weren't explicitly discussed. lmk what you think.

my main motivation was to make it clear how to use each command, so hopefully, the "required" vs "optional" flag sections make sense to you. i did this as an alternative to moving certain flags to positionals. the features for list and retrieve seem worthwhile imo since they give the dev more flexibility on how to use them. the --version flag is also a nice feature imo because i found myself trying to do this multiple times—but i might have to revisit the logic because it looks like it was an unverified commit via the GH bot?

lastly, i renamed the wallet command to account—idk if this makes sense to you, but i describe my reasoning above. so, feel free to let me know if i should revert that or anything else here!

Copy link
Contributor

@brunocalza brunocalza left a comment

Choose a reason for hiding this comment

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

Awesome work 👏

In general, I agree with all proposed changes. They make sense to me. Nice work on improving the HELP, adding aliases and improving descriptions 💯

@@ -0,0 +1,31 @@
name: Update version file
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! ideally, the binaries workflow would depend on a successful run of this one. not sure how easy it is to implement that, i think it's fine the way it is

Copy link
Contributor Author

@dtbuchholz dtbuchholz Dec 19, 2023

Choose a reason for hiding this comment

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

@brunocalza maybe this would work? https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

name: Generate binaries
on:
  workflow_run:
    workflows: ["Update version file"]
    types:
      - completed

i have the update-version workflow to only work on main, though, so would that pose an issue if the generated binaries workflow was dependent on it?

but yeah, maybe we can figure that out in a separate PR, if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

hum, I missed this was triggered when a push happens to main. Ideally, it would be triggered when a release is created, right? and the binaries would be run after that. because if you create version.json when a push happens, the next binary release will have the wrong version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brunocalza ah, i see. when the binaries are generated, it looks like that's where the tag gets created, which is what i fetch to get the version in the CLI.

  • publishing
    • scm releases
      • creating or updating release                 tag=v0.0.6 repo=tablelandnetwork/basin-cli
      • release updated                              name=v0.0.6 release-id=133993550 request-id=3BC0:2FB0:182D22B:1919694:657B5041
      • uploading to release                         file=dist/basin-darwin-amd64.tar.gz name=basin-darwin-amd64.tar.gz
      • uploading to release                         file=dist/basin-linux-arm64.tar.gz name=basin-linux-arm64.tar.gz
      • uploading to release                         file=dist/basin-linux-amd64.tar.gz name=basin-linux-amd64.tar.gz
      • uploading to release                         file=dist/basin-darwin-arm64.tar.gz name=basin-darwin-arm64.tar.gz
      • published                                    url=https://github.com/tablelandnetwork/basin-cli/releases/tag/v0.0.6
      • took: 4s

hm, so maybe my approach won't work because i'd need my GH action to run after that tag is created but before the binaries are generated?

Copy link
Contributor

Choose a reason for hiding this comment

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

The tag is not being created by the workflow. The tag is created by Github itself when a release is created.

I think you need to switch to

on:
  release:
    types:
      - created

and somehow get access to the tag being created inside the workflow.

You can test that, by drafting new releases targeting your branch.

If this is taking too much of your time. Feel free to merge it and I can figure that out for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brunocalza yeah, if you could take a look, that might be best! i dont want to mess up anything there. should i just merge this into your branch now?

i did alter my version workflow to take your trigger into account—this seems like it should work, but i havent tested it:

name: Update version file

on:
  release:
    types:
      - created

jobs:
  update_version:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
        with:
          fetch-depth: 0

      - name: Update version.json
        run: |
          echo "{
            \"version\": \"${{ github.event.release.tag_name }}\"
          }" > version.json

      - name: Commit changes
        uses: stefanzweifel/git-auto-commit-action@v4
        with:
          commit_message: "chore: update version to ${{ github.event.release.tag_name }}"

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 feel free to merge it

README.md Outdated Show resolved Hide resolved
@dtbuchholz dtbuchholz merged commit 9ca46de into bcalza/rename Dec 19, 2023
2 checks passed
@dtbuchholz dtbuchholz deleted the dtb/cli-positionals branch December 19, 2023 20:06
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