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.

Sign up for GitHub

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

Jump to bottom

[PLATFORM-633] Migrate commands left to builder #140

Merged
merged 26 commits into from
May 11, 2021
Merged

[PLATFORM-633] Migrate commands left to builder #140

merged 26 commits into from
May 11, 2021

Conversation

raulb
Copy link
Member

@raulb raulb commented May 6, 2021 •
edited
Loading

Description of change

These commands do conform the new design, but to consolidate an only way to write commands, these are also being migrated to thew new builder.

While I'm at it, I'll fix some of the things there were in the backlog and will add tests.

Fixes https://meroxa.atlassian.net/browse/PLATFORM-546
Fixes https://meroxa.atlassian.net/browse/PLATFORM-633
Fixes https://meroxa.atlassian.net/browse/PLATFORM-461 (once we're in V2)

Commands migrated

  • api
  • login (auth)
  • logout (auth)
  • whoami (auth)
  • billing
  • open
  • version

Type of change

  • New feature
  • Bug fix
  • Refactor
  • Documentation

How was this tested?

  • Unit Tests
  • Tested in staging

Demo

Before this pull-request

❯ meroxa api get /v1/resources
> get /v1/resources
< 400 Bad Request HTTP/2.0
< Date Thu, 06 May 2021 14:55:03 GMT
< Content-Type text/html
< Content-Length 122
< Server awselb/2.0
<html>
<head><title>400 Bad Request</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
</body>
</html>

❯ .m help
  ...
  login       Login or Sign up to the Meroxa Platform
  logout      Clears local login credentials of the Meroxa Platform
  ...
  whoami      Display the current logged in user

After this pull-request

❯ .m api get /v1/resources
> GET /v1/resources
< 200 OK HTTP/2.0
< Date Thu, 06 May 2021 14:55:17 GMT
< Content-Type application/json; charset=utf-8
< Content-Length 487
< Meroxa-Request-Id cbfc9316-1eba-4fd9-a607-4add7d12efd2
< Vary Origin
[]

❯ MEROXA_V2=only .m help
The Meroxa CLI allows quick and easy access to the Meroxa Data Platform.

Using the CLI you are able to create and manage sophisticated data pipelines
with only a few simple commands. You can get started by listing the supported
resource types:

meroxa resources list --types

Usage:
  meroxa [command]

Available Commands:
  ...
  auth        Authentication commands for Meroxa
  ...

Flags:
      --config string      config file
      --debug              display any debugging information
  -h, --help               help for meroxa
      --json               output json
      --timeout duration   set the client timeout (default 10s)

Use "meroxa [command] --help" for more information about a command.

❯ MEROXA_V2=only .m help auth
Authentication commands for Meroxa

Usage:
  meroxa auth [command]

Available Commands:
  login       Login or Sign up to the Meroxa Platform
  logout      Clears local login credentials of the Meroxa Platform
  whoami      Display the current logged in user


Flags:
  -h, --help   help for auth

Global Flags:
      --config string      config file
      --debug              display any debugging information
      --json               output json
      --timeout duration   set the client timeout (default 10s)

Use "meroxa auth [command] --help" for more information about a command.

@raulb raulb requested a review from lovromazgon May 6, 2021 16:24
@raulb raulb marked this pull request as ready for review May 6, 2021 16:25
@raulb raulb force-pushed the raul/migrate-to-builder branch from 45b3641 to 5dc7174 Compare May 6, 2021 19:27
@raulb raulb force-pushed the raul/migrate-to-builder branch from 5dc7174 to 6f285b0 Compare May 6, 2021 19:30
@raulb raulb force-pushed the raul/migrate-to-builder branch from 71b572c to 757274a Compare May 6, 2021 20:44
@raulb raulb force-pushed the raul/migrate-to-builder branch from ec8faf2 to 2a2d953 Compare May 7, 2021 10:26
lovromazgon
lovromazgon requested changes May 7, 2021
View reviewed changes
Copy link
Member

@lovromazgon lovromazgon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've found a bug - when logging in on a fresh machine (no auth.meroxa.io cookie) the auth0 login page opens but after logging in the redirect page doesn't open anything, since the server is not running anymore, the CLI process is already stopped at that point.

cmd/meroxa/root/api/api.go
a.logger.Infof(ctx, "< %s %s\n", k, strings.Join(v, " "))
}

a.logger.Info(ctx, prettyJSON.String())
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add a JSON output to enable the --json flag? I imagine it would be quite useful to have in this command. Although the JSON logger right now marshals the output, while in this case we would need to just log the response body as it is, without running it through the JSON marshaler.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lovromazgon thought about that, but I wondered if that could be useful. Glad you thought the same, so I'll make that change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lovromazgon let me know what you think d8511c9

@raulb raulb requested a review from lovromazgon May 8, 2021 14:00
@raulb
Copy link
Member Author

raulb commented May 8, 2021

@lovromazgon made some changes. Let me know what you think now. Thank you!

@raulb raulb force-pushed the raul/migrate-to-builder branch from c303a5f to b55efd6 Compare May 9, 2021 12:16
@raulb raulb mentioned this pull request May 11, 2021
Merged
8 tasks
@raulb raulb merged commit 29f538a into master May 11, 2021
@raulb raulb deleted the raul/migrate-to-builder branch May 11, 2021 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lovromazgon lovromazgon lovromazgon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@raulb @lovromazgon