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: migrate rover config whoami output to stdout and add json format #1560

Conversation

scombat
Copy link
Contributor

@scombat scombat commented Apr 2, 2023

Hello everyone,

I am open to collaborating with you to improve this project. I welcome feedback and suggestions on how to make the code more efficient and effective. Please let me know if you have any thoughts or ideas for improvement. I understood that not all changes may be accepted or merged due to the triage label, and that's perfectly fine. I'm open to feedback and willing to make changes as needed to improve the codebase.
I would like to add functional tests for common user use cases, as well as for graphs with valid and invalid profiles for each case. However, I am not sure if it is relevant or if you have already implemented functional tests.

Description

BREAKING CHANGE: This pr fixes #1380 by using a RoverOutput whoami variant to print the output of the rover config whoami command to stdout instead of stderr. It also adds the capability to print the output as json with the --json flag.
This PR migrates the output of rover config whoami command from stderr to stdout and fixes #1380. Additionally, this PR adds the --json flag to allow users to retrieve the output in JSON format. This change improves the usability of the rover CLI and enhances the user experience.

Related Issues & discussions

Related PRs

WIP :

  • Migrate the output of rover config whoami to stdout using RoverOutput::ConfigWhoAmIOutput variant
  • Add a table format to improve the readability of the output
  • Implement the Display trait for the Actor enum to print the actor type in a human-readable format
  • Allow users to retrieve the output in JSON format
  • Add unit tests for the new methods
  • Add functional tests for error cases and different output formats
  • Add documentation

Checklist

  • I have added tests that prove my fix is effective or that my feature works.
  • I have added or updated necessary documentation.
  • I sign the CLA. (scombat)
  • I have checked that no one else is working on similar changes.
  • I have read the latest CONTRIBUTING
  • I tested my code with cargo test --workspace and all tests passed locally.
  • I used cargo clippy to check for common mistakes and code style issues.
  • I used cargo fmt --all to format my code.

Outputs :

User Actor :

Human readable output (Keys are displayed in Style::WhoAmIKey green but not in images below) [updated]

ray-so-export (2)

Json format (with --format json flag) [updated]

ray-so-export (3)

Graph Actor :

Human readable output (Keys are displayed in Style::WhoAmIKey green but not in images below) [updated]

ray-so-export

Json format (with --format json flag) [updated]

ray-so-export (4)

@scombat scombat changed the title wip!: migrate rover config whoami output to stdout and add json format wip!: (need help) migrate rover config whoami output to stdout and add json format Apr 8, 2023
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

Requested some small changes

src/command/output.rs Outdated Show resolved Hide resolved
src/command/output.rs Outdated Show resolved Hide resolved
@EverlastingBugstopper
Copy link
Contributor

@scombat thanks so much for this PR. It's looking really good already and I really appreciate the level of detail that you've included when filing this. I don't think you need to add any more tests than you already have, and I also don't think we need to update documentation because base functionality isn't changing. We'll just make sure to call this out as a breaking change in the changelog - since Rover is still pre-1.0 this is reasonable.

refactor(whoami): extract conditions in helper methods

to improve tests, lisibility and maintenability

test(whoami): add unit tests for actor type guard

test(whoami): add unit tests for masked or unmasked api key

test(whoami): add unit tests for graph title helper

test(whoami): add unit tests for uniq graph id

test(whoami): add uni tests for user id
feat(RoverOutput)!: add config whoami json output

feat(RoverOuput): add WhoAmI variant
to improve whoami command output readability
in RoverOuput::ConfigWhoAmIOuput variant

refactor(output): move the graph title below the graph ID for diplay

refactor(output): reorder ConfigWhoAmIOutput fields alphabetically

refactor(output): replace "Unique Graph ID" displayed key by "Graph ID"

for the human readable format output.

refactor(output): replace "uniq_graph_id" property name with "graph_id"

for the json format output.
refactor(whoami): rename uniq_graph_id field to graph_id in command output

refactor(whoami): reorder command output fields alphabetically
@scombat scombat force-pushed the fix/1380-migrate-whoami-output-to-stdout branch from a2c4ea7 to 426670b Compare April 12, 2023 12:31
@scombat scombat changed the title wip!: (need help) migrate rover config whoami output to stdout and add json format wip!: migrate rover config whoami output to stdout and add json format Apr 12, 2023
@scombat
Copy link
Contributor Author

scombat commented Apr 12, 2023

Hi @EverlastingBugstopper,

I wanted to thank you for taking the time to review this PR and for providing such helpful feedback. Your message was really encouraging and I appreciate the level of detail you've given in your comments. I'm glad to hear that the current tests and documentation are sufficient for this change.

Once again, thank you for your support and valuable time. Please don't hesitate to let me know if there are any further changes or improvements that you think would be useful or if I need to remove the wip PR title's prefix and make this PR ready for review.

Best regards, Steve.

Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

Thanks!

@EverlastingBugstopper EverlastingBugstopper marked this pull request as ready for review April 12, 2023 16:00
@EverlastingBugstopper EverlastingBugstopper changed the title wip!: migrate rover config whoami output to stdout and add json format feat: migrate rover config whoami output to stdout and add json format Apr 12, 2023
@EverlastingBugstopper EverlastingBugstopper added the BREAKING ❗ a PR that represents a breaking change label Apr 12, 2023
@EverlastingBugstopper EverlastingBugstopper added the feature 🎉 new commands, flags, functionality, and improved error messages label Apr 12, 2023
@EverlastingBugstopper EverlastingBugstopper added this to the vNext milestone Apr 12, 2023
@EverlastingBugstopper EverlastingBugstopper merged commit 6418eff into apollographql:main Apr 12, 2023
8 checks passed
EverlastingBugstopper added a commit that referenced this pull request Apr 19, 2023
# [0.14.0] - 2023-04-19

> Important: 1 potentially breaking changes below, indicated by **❗
BREAKING ❗**

## ❗ BREAKING ❗

- **`rover config whoami` outputs to `stdout` instead of `stderr` and
using `--format json` includes more information than success or failure
- @scombat, #1560 fixes #1380**

When running `rover config whoami`, the output will print to `stdout`
instead of `stderr`. This may break scripts that relied on parsing the
output from `stderr`. The good news is that these scripts should be
easier to write because passing `--format json` to `rover config whoami`
will print structured output that can be parsed with a tool like
[`jq`](https://stedolan.github.io/jq/tutorial/).

## 🚀 Features

- **ALlow custom headers when running introspection with `rover
supergraph compose` - @dbanty, #1574 fixes #615**

A new field is available in `supergraph.yaml` files that allows sending
headers along with introspection. This value also supports environment
variable interpolation for sensitive values like authentication tokens.

- **Print a wanring when attempting to publish a subgraph with an
invalid routing URL - @trevor-scheer, #1543 fixes #1477**

When running `rover subgraph publish`, if the `--routing-url` you
specify or the routing URL stored in GraphOS is unroutable, a warning
will be printed. If you are not in CI, you will need to manually confirm
the publish to continue. You can dismiss the warning by passing
`--allow-invalid-routing-url`.

  **Note:** This warning will become a hard error in the future.

## 🐛 Fixes

- **Spawn a thread to avoid a rare deadlock in `rover dev` -
@EverlastingBugstopper, #1548 fixes #1544**

## 🛠 Maintenance

- **Updates dependencies - @EverlastingBugstopper, #1562**

  `apollo-parser` 0.4 -> 0.5
  `git2` 0.16 -> 0.17
  `opener` 0.5 -> 0.6
  `predicates` 2 -> 3
  `serial_test` 1 -> 2
  `toml` 0.5 -> 0.7
  ~`crossterm`~

- **Use Apple Silicon in CI - @EverlastingBugstopper, #1557 fixes
#1555**

There should be no user facing change here, we just run builds in CI
much faster.

## 📚 Documentation

- **Adds Apollo CLI migration guide to Rover docs - @StephenBarlow,
#1568**

The (deprecated) Apollo CLI documentation and the migration guide for
Rover now live in Rover's docset.

- **Cleans up nomenclature and links in Rover docs - @StephenBarlow,
#1571 and #1573**

Rover's documentation has been updated to refer to the [new GraphOS
documentation](https://www.apollographql.com/docs/graphos) along with
updating some terminology.

- **Mention community-maintained installation methods - @dbanty, #1542**

Rover's documentation now mentions the unofficial installation methods
`nix` and `brew`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING ❗ a PR that represents a breaking change feature 🎉 new commands, flags, functionality, and improved error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rover config whoami should print to stdout instead of stderr and return JSON data
2 participants