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

feat(cargo-shuttle): raw table output, fix table column alignment #1319

Merged
merged 1 commit into from Oct 24, 2023
Merged

feat(cargo-shuttle): raw table output, fix table column alignment #1319

merged 1 commit into from Oct 24, 2023

Conversation

supleed2
Copy link
Contributor

@supleed2 supleed2 commented Oct 12, 2023

Description of change

How has this been tested? (if applicable)

The following commands were run, inspecting the output manually: (nano is a project I have running)

  • cargo run --bin cargo-shuttle -- --api-url https://api.shuttle.rs deployment list --name nano
  • cargo run --bin cargo-shuttle -- --api-url https://api.shuttle.rs deployment list --name nano --raw
  • cargo run --bin cargo-shuttle -- --api-url https://api.shuttle.rs project list
  • cargo run --bin cargo-shuttle -- --api-url https://api.shuttle.rs project list --raw
  • cargo run --bin cargo-shuttle -- --api-url https://api.shuttle.rs resource list --name nano
  • cargo run --bin cargo-shuttle -- --api-url https://api.shuttle.rs resource list --name nano --raw
  • cargo run --bin cargo-shuttle -- --api-url https://api.shuttle.rs secrets --name nano
  • cargo run --bin cargo-shuttle -- --api-url https://api.shuttle.rs secrets --name nano --raw

Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

Looking good at a glance. Will try it out later. Anything else to add?

@supleed2 supleed2 marked this pull request as ready for review October 13, 2023 09:49
@supleed2
Copy link
Contributor Author

Just wanted to look though and make sure I didn't miss anything, I'm happy with it yeah.

Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@jonaro00
Copy link
Member

I see that the raw project list will use colors, but not the deployment list. I think colorless is preferred for raw table. Is this a quick fix or is it complicated due to #962 ?

@supleed2
Copy link
Contributor Author

I see that the raw project list will use colors, but not the deployment list. I think colorless is preferred for raw table. Is this a quick fix or is it complicated due to #962 ?

The project list only applies colours when raw is false, or did I miss something? 😅

@jonaro00
Copy link
Member

@supleed2
image

@supleed2
Copy link
Contributor Author

supleed2 commented Oct 20, 2023

I've rebased my new changes on master so it can be more easily merged. The changes should now close #1315 and also close #962. Thanks for noticing the coloured output @jonaro00

Edit: I see the failing tests, I'm going through them now

Edit 2: Took a few tries, but looks like it went better this time :D

@supleed2 supleed2 changed the title feat(cargo-shuttle): option to output raw tables feat(cargo-shuttle): raw table output, fix(cargo-shuttle): table columns Oct 20, 2023
Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

Great job! 🥳
I simplified some unnecessary complexity. I'll let @oddgrd check one final time.

@jonaro00 jonaro00 changed the title feat(cargo-shuttle): raw table output, fix(cargo-shuttle): table columns feat(cargo-shuttle): raw table output, fix table column alignment Oct 20, 2023
@oddgrd
Copy link
Contributor

oddgrd commented Oct 23, 2023

This looks great, but we are now missing the color on the project state in commands like cargo shuttle status and cargo shuttle project stop/start`, which I thought was quite nice. I assume it's because we applied the color for project states everywhere, and that is what was causing the misalignment bug, but is there any chance we can get the color back for these other commands?

@supleed2
Copy link
Contributor Author

I'm going through the commands to check, but the output from cargo shuttle status is still coloured, due to these lines: https://github.com/shuttle-hq/shuttle/blob/main/common/src/models/service.rs#L44-L47

I think I've managed to restore colour to the rest of the commands, so I'll add that to my commit and rebase on main again in a bit. Thanks again for the spot!

@jonaro00 I also noticed while going through the commands that cargo shuttle resource list and cargo shuttle secrets have table output, should I add a --raw flag for those commands too?

Flag (`--raw`) added to `deployment list`, `project list`, `resource list` and `secrets` commands
Renamed `get_table`s to `get_projects_table`/`get_secrets_table` for consistency
Project state returns `&str` like Deployment state
All output responds to `--raw` flag
Project state `impl Display` does not style output, styling done in calling code
Simplify output message formatting
@supleed2
Copy link
Contributor Author

I decided to implement the --raw flag for resource list and secrets as well as rebasing on main, hopefully no merge conflicts arise later :)

This did include fixing a couple merge issues from the new project delete command, but I think it looks right now. And the status and project status commands now output coloured statuses almost as before, the whole message is coloured rather than just the first word for operations with counts.

@jonaro00 jonaro00 merged commit cf7bcf7 into shuttle-hq:main Oct 24, 2023
33 checks passed
@oddgrd
Copy link
Contributor

oddgrd commented Oct 24, 2023

Excellent work, thank you! ❤️ By the way, in the future, it's preferable if you don't squash your commits after the review process has started, since it makes it harder for us to distinguish the changes you made to address reviews. We will squash them when we merge the PR.

@supleed2 supleed2 deleted the raw-table-output branch October 24, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants