-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[RFC] feat: add base code for the image of Appsody LoopBack stack #4250
Conversation
b7d835e
to
0ae8a13
Compare
0ae8a13
to
0ef5995
Compare
@raymondfeng , I have two questions:
:) |
@emonddr Good questions. Yes, Appsody stacks by design break down an application into two parts:
Developers can only change the 2nd part and it's packaged with the base into a Docker container by Appsody as it goes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I agree it's more convenient for us to keep the Appsody template in our monorepo.
I have few major concerns though:
-
Duplication of content between
lb4 app
and Appsody templates. Besides the difficulties of keeping these two templates in sync, it's also not clear to me which parts are intentionally different. -
How are we going to test the correctness of the template and verify integration with Appsody?
-
What impact will the Appsody-related test suite have on duration of
npm test
(local development) and Travis CI jobs? Our tests are already taking too long to run, we must be mindful about introducing more long-running tests.
await app.stop(); | ||
}); | ||
|
||
it('invokes GET /ping', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a good idea to include the /ping
enpoint in the base stack? Isn't this just a dummy endpoint useful for testing, but not suitable for deployment to production?
/dist | ||
|
||
# Cache used by TypeScript's incremental build | ||
*.tsbuildinfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned about the duplication of .gitignore
content. Can we find a way how to produce this file as part of build and/or at publish time? I would expect that most of the ignore rules should be the same as we are using for new projects created by lb4 app
.
The same concern & comment applies to other parts where the Appsody template is duplicating the output of lb4 app
.
Instead of committing the code to git, have you considered introducing a new CLI option or command to scaffold the Appsody-enabled application? If Appsody is not able to run a 3rd-party script as part of project bootstrapping, then perhaps we can run that script ourselves to create files to be published for Appsody consumption? For example, the instructions for updating the Appsody template can look like this:
|
@raymondfeng , another thing I noticed is that the port 3000 seems to be hard-coded in the stack/template. Even if I change the port number in the rest server application config, I can see docker commands still using 3000. |
Since Appsody development has ended, should we close this pull request? |
This is an attempt to extract code to
loopback-next
repo from https://github.com/appsody/stacks/tree/master/incubator/nodejs-loopback. The move will make maintenance of the LoopBack Stack for Appsody much simpler.Items for discussion:
Can we use a different repo than
appsody
- https://github.com/appsody/stacks/blob/master/index.yaml#L61?This PR creates a module as the base for appsody/loopback applications - we can release it independent of the stack.
What about the template? Can it be hosted on our repo and referenced via https://github.com/appsody/stacks/blob/master/index.yaml#L61?
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈