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: apps deploy improvements #301

Merged
merged 9 commits into from
Apr 8, 2022
Merged

feat: apps deploy improvements #301

merged 9 commits into from
Apr 8, 2022

Conversation

raulb
Copy link
Member

@raulb raulb commented Apr 5, 2022

Description of change

Fixes https://github.com/meroxa/turbine-project/issues/132

  • It also makes some changes on some other build commands.
  • It avoids printing out an empty line when there's nothing to log out: 488cf68
  • It shows a friendlier message when there's an invalid token: 1c8cc6b
  • It adds spinner through the deployment process
  • It adds a way to include spinners across the code as part of logger
  • It updates the deployment output

Type of change

  • New feature
  • Bug fix
  • Refactor
  • Documentation

How was this tested?

  • Unit Tests
  • Tested in staging
  • Tested in minikube

Demo

See slack! I recorded a couple.

@raulb raulb self-assigned this Apr 5, 2022
@raulb raulb changed the base branch from master to update-turbine-pkg April 5, 2022 13:02
@raulb raulb force-pushed the raul/apps-deploy-combined branch 5 times, most recently from 73e67f4 to e22baef Compare April 5, 2022 14:59
Base automatically changed from update-turbine-pkg to master April 6, 2022 10:15
@raulb raulb force-pushed the raul/apps-deploy-combined branch 2 times, most recently from 1c8cc6b to 7d52615 Compare April 6, 2022 11:32
cmd/meroxa/turbine_cli/utils.go Outdated Show resolved Hide resolved
@raulb raulb force-pushed the raul/apps-deploy-combined branch 4 times, most recently from 93262fc to 520aa58 Compare April 7, 2022 15:03
@raulb raulb marked this pull request as ready for review April 7, 2022 15:09
@raulb raulb requested review from ericcheatham and jmar910 April 7, 2022 15:09
@raulb raulb changed the title chore: Refactor apps deploy so could be utilized by any language feat: apps deploy improvements Apr 7, 2022
@raulb raulb force-pushed the raul/apps-deploy-combined branch from 520aa58 to c347110 Compare April 7, 2022 15:10
- chore: Make dockerhub deployment generic
- fix: Feature flag error
- chore: Small refactor on build cmds
- feat: Include how to inspect logs of failed build
- fix: Avoid empty lines when not necessary
- feat: Improve error message when invalid token

before:

```
$ meroxa anything
Error: Post "https://api.meroxa.io/v1/sources": oauth2: cannot fetch token: 403 Forbidden
Response: {"error":"invalid_grant","error_description":"Unknown or invalid refresh token."}
```

after:

```
$ meroxa anything
Error: unknown or invalid refresh token, please run `meroxa login` again
```
- feat: Add spinner to logger
- feat: Include language on deploy output
- feat: Add language validation
- feat: include dashboard url
@raulb raulb force-pushed the raul/apps-deploy-combined branch from c347110 to aa21e98 Compare April 7, 2022 15:21
@raulb raulb requested a review from janelletavares April 7, 2022 16:29
@janelletavares
Copy link
Contributor

janelletavares commented Apr 8, 2022

Screenshot from 2022-04-07 18-14-17

the something-bad-happened emojis don't look like anything in linux. i'm using bash. my editor just has an empty block where they should be, as well.


d.path, err = turbineCLI.GetPath(d.flags.Path)
if err != nil {
return err
}
d.appName = path.Base(d.path)
Copy link
Contributor

Choose a reason for hiding this comment

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

this wasn't true with the example apps. I'm in the process of updating them, however.

@janelletavares
Copy link
Contributor

@raulb I'm finished reviewing and testing. Let me know if you want to discuss anything that I pushed in my commits.

cmd/meroxa/turbine_cli/utils.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jayjayjpg jayjayjpg left a comment

Choose a reason for hiding this comment

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

This reads great! I tested this in combination with the turbine-js branch james/fix-deploy-past-n-future and this seems to return an error when using the meroxa apps deploy command when deploying a JS app

I'm unsure if this is related to the Turbine lib or the CLI changes though, what do you think?

Deploy with --path

initappsjs

Deploy from current directory

initappsjs2

initappsjs3

@jayjayjpg jayjayjpg self-requested a review April 8, 2022 11:01
Copy link
Contributor

@jayjayjpg jayjayjpg left a comment

Choose a reason for hiding this comment

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

I just noticed after looking into this more that this is working as expected and that app deployments only fail under certain circumstances unrelated to this change

This reads great and works well, thank you for the super swift fix! ✨

jsinitagain

Just a heads up, that I tracked the unrelated issue as a bug ticket here

@raulb raulb merged commit 4111970 into master Apr 8, 2022
@raulb raulb deleted the raul/apps-deploy-combined branch April 8, 2022 13:01
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