search

eclipse-archived / codewind

The official repository of the Eclipse Codewind project
https://codewind.dev
Eclipse Public License 2.0
113 stars 45 forks source link

Appsody CLI update causes Portal to crash and Jenkins tests to fail #946

Closed james-wallis closed 4 years ago

james-wallis commented 4 years ago

Codewind version: Latest OS: MacOS and Jenkins

Description: Portal API /api/v1/project-types causes PFE to crash. When the api is called it runs the command ${__dirname}/appsody repo list -o json using exec to get a list of repositories from the Appsody CLI. Looks like Appsody has been updated and doesn't give the correct output in JSON so the line const json = JSON.parse(stdout); fails and causes Portal to crash.

The stout from running the command above in PFE gives the output:


*
*
*

A new CLI update is available.
Please go to https://appsody.dev/docs/getting-started/installation#upgrading-appsody and upgrade from 0.4.7 --> 0.4.8.

*
*
*

{"apiVersion":"v1","generated":"2019-11-01T11:56:18.6478923Z","repositories":[{"name":"appsodyhub","url":"https://github.com/appsody/stacks/releases/latest/download/incubator-index.yaml","default":true},{"name":"experimental","url":"https://github.com/appsody/stacks/releases/latest/download/experimental-index.yaml"}]}

and the error given when Portal crashes is:

undefined:2
*
^

SyntaxError: Unexpected token * in JSON at position 1
    at JSON.parse (<anonymous>)
    at exec (/codewind-workspace/.extensions/codewind-appsody-extension/templatesProvider.js:33:35)
    at ChildProcess.exithandler (child_process.js:285:7)
    at ChildProcess.emit (events.js:198:13)
    at maybeClose (internal/child_process.js:982:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:259:5)

Steps to reproduce: Pull latest. ./run.sh curl http://127.0.0.1:{PORTALS_PORT}/api/v1/project-types

Workaround:

james-wallis commented 4 years ago

Looks like the issue is in this file: https://github.com/eclipse/codewind-appsody-extension/blob/master/templatesProvider.js#L47

I'm aware this is an Appsody issue but we should ensure that we don't crash if the output isn't in JSON.

james-wallis commented 4 years ago

I've raised this with Appsody here: https://github.com/appsody/appsody/issues/563.

We should still at least add a try/catch when parsing JSON from stdout.

jopit commented 4 years ago

If the appsody binaries we ship in Codewind will output non-valid json when they detect there's a newer version of appsody available, isn't that going to break existing customers using 0.5.0 @sghung @makandre ?

makandre commented 4 years ago

I'll look into it. I had changed the timestamp in appsody config so it would not do that version check....

makandre commented 4 years ago

So it was a combination of factors of:

james-wallis commented 4 years ago

@makandre I've just seen the PR you've raised. I agree it would fix this but if a check for valid JSON is not added then we run the risk of this issue appearing in production if Appsody release a new version?

@jopit I think this is a major bug as we can't get any PR's into Codewind.

makandre commented 4 years ago

True... I can put in a check for valid JSON in a follow-up PR

Tho in theory after 0.4.8, it would respect us setting the timestamp so that it would never do the version check

james-wallis commented 4 years ago

I agree with the theory but as its running an exec command and assuming it returns a JSON object it would be safer to add the valid JSON check. It just protects the off chance that something other than valid JSON is outputted.

james-wallis commented 4 years ago

@sghung can you re-open this until the fix has been verified in this repo please.

makandre commented 4 years ago

All fixes for this are in now

james-wallis commented 4 years ago

Happy this is guarded against happening again due to https://github.com/eclipse/codewind/pull/949