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

Define contract for turbine response and capture #314

Merged
merged 1 commit into from
Apr 15, 2022

Conversation

jmar910
Copy link
Contributor

@jmar910 jmar910 commented Apr 13, 2022

Description of change

CLI now expects a specific format when receiving responses from turbine-js. This allows us to only capture what we care about and leave the rest of the output alone.

Type of change

  • New feature
  • Bug fix
  • Refactor
  • Documentation

How was this tested?

  • Unit Tests
  • Tested in staging
  • Tested in minikube

Demo

before after

Additional references

See this thread https://meroxa.slack.com/archives/C02L14X6TA9/p1649879600763529

Blockers

return processError(err, path, output)
}

r, _ := regexp.Compile("\nturbine-response: (true|false)\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
r, _ := regexp.Compile("\nturbine-response: (true|false)\n")
r := regexp.MustCompile("\nturbine-response: (true|false)\n")

nit

@jmar910 jmar910 force-pushed the james/harden-ouput-capturing branch from a7ef81d to 488f490 Compare April 14, 2022 08:52
@jmar910 jmar910 force-pushed the james/harden-ouput-capturing branch from 488f490 to 1943e97 Compare April 14, 2022 08:53
@jmar910 jmar910 marked this pull request as ready for review April 14, 2022 09:04
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 took a spin on this locally and noticed that app initialization and runs work great with regards to output and error handling, but something seems amiss with deploying apps -- is this related to this change?

$ ./meroxa apps deploy --path ../demos/myjsapp
Checking for uncommitted changes...
	✔ No uncommitted changes!
Validating branch...
	✔ Deployment allowed from master branch!
Preparing application "myjsapp" (javascript) for deployment...
	✔ Application built!
	
Error: strconv.ParseBool: parsing "turbine-response: true": invalid syntax

The recording includes

  • a failed deployment of an app instance to the same namespace myjsapp
  • another subsequent redeployment to the myjsapp after deleting the previous app that also fails

democlioutput

@jmar910
Copy link
Contributor Author

jmar910 commented Apr 14, 2022

@jayjayjpg just to confirm, did you pull and build the latest james/update-response-format branch from https://github.com/meroxa/turbine-js/pull/58 when you tested this locally? I forgot to add it in this PR template, but these two PRs have to be merged at the same time

@jayjayjpg
Copy link
Contributor

@jmar910 Yes exactly, but let me double check and record just to make sure, also happy to huddle if it's helpful!

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.

After @jmar910 paired with me we noticed that I failed to pull the latest master before testing and this change is already working as expected -- awesome work!

democlioutput3

@jmar910 jmar910 merged commit 497afc4 into master Apr 15, 2022
@jmar910 jmar910 deleted the james/harden-ouput-capturing branch April 15, 2022 04:51
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.

3 participants