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

update starter project #1

Merged
merged 5 commits into from
Jun 30, 2020
Merged

Conversation

skoh7645
Copy link

This PR updates the starter project to use the best practices as discussed with the Node team

server.js Outdated

app.get('/', (req, res) => {
// Use req.log (a `pino` instance) to log JSON:
req.log.info({message: 'Hello from NodeJS Starter Application!'});
Copy link

Choose a reason for hiding this comment

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

Change NodeJS to Node.js

server.js Outdated
app.get('/', (req, res) => {
// Use req.log (a `pino` instance) to log JSON:
req.log.info({message: 'Hello from NodeJS Starter Application!'});
res.send('Hello from NodeJS Starter Application!');
Copy link

Choose a reason for hiding this comment

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

Change NodeJS to Node.js

@@ -0,0 +1,15 @@
{
Copy link

Choose a reason for hiding this comment

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

Is this vscode specific metadata file which has environment specific port number info supposed to be part of the project?

Copy link
Author

@skoh7645 skoh7645 Jun 30, 2020

Choose a reason for hiding this comment

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

We had this file in the Appsody nodejs templates, we thought it might be easier for the user as they wouldn't have to create it themselves. But i do see where you are coming from and we should keep it uniform across all stack starter projects so we've removed it

Choose a reason for hiding this comment

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

This is interesting one.. User will need to know reoteRoot path to eb able to setup this themselves.
@elsony do we document this somewhere today? I remember there was a field added to 2.0 spec for pointing to stack documentation. Should we look to use that for capturing such info?

Copy link
Author

Choose a reason for hiding this comment

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

The attach config doesn't actually require the remoteRoot path, it is enough to give it the correct port, I was just trying to be "helpful". For example, the following debugging config also works for me:

    "version": "0.2.0",
    "configurations": [
        {
            "type": "node",
            "request": "attach",
            "name": "Attach Program",
            "skipFiles": [
                "<node_internals>/**"
            ],
            "port": 5858,
        }
    ]
}

Copy link
Author

Choose a reason for hiding this comment

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

That config is largely generated by vscode, I just changed the request from "launch" to "attach" and added the port

Copy link

Choose a reason for hiding this comment

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

@neeraj-laad For your question on doc, any stack/devfile specific doc should go to the website under the metadata on a devfile. For info specific to the starter project, it should go to the README file in the project.

Copy link

@elsony elsony left a comment

Choose a reason for hiding this comment

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

Thanks for the update!
LGTM

@elsony elsony merged commit e832173 into odo-devfiles:master Jun 30, 2020
@skoh7645 skoh7645 deleted the updatestarter branch June 30, 2020 17:05
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