-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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 README.md #1641
Update README.md #1641
Conversation
To reflect the fact that you can now sensibly deploy from a package (ie. be able to configure the app). Make the first thing be downloading a prebuilt package so people who only read the first part don't end up running npm start in production and complain about needing npm.
1. Wait a few seconds for the initial build to finish (the command won't | ||
terminate: it's running a web server for you). | ||
1. Open http://127.0.0.1:8080/ in your browser to see your newly built Vector. | ||
1. Configure the app by modifying the `config.json` file (see below for details) |
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.
need to copy the sample to start with?
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.
and should this be after the next step, since the config.json doesn't go in the tarball?
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.
config.json does now go in the tarball since it's fetched by the app
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.
Hrm. Should it, though? My expectation would be that the config would not be in the tarball.
ptal |
=========== | ||
|
||
Configure the app by modifying the `config.json` file: | ||
You can configure the app by copying the sample and modifying the `config.json` file: |
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.
Need to point out that these are in the vector
subdirectory.
1. Wait a few seconds for the initial build to finish (the command won't | ||
terminate: it's running a web server for you). | ||
1. Open http://127.0.0.1:8080/ in your browser to see your newly built Vector. | ||
1. Configure the app by copying `config.sample.json` to `config.json` and modifying |
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.
Repeating here since the original comment is on an outdated diff:
I'm not convinced it is correct to ship the config.json in the tarball.
OTOH that's a separate issue to what's going on here.
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.
True - we could put it in the root directory, although then it's unclear that it needs to be copied to the vector/
directory. Another option would be to exclude config.sample.json
from the tarball explicitly.
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 assume you mean config.json
. The sample /should/ be shipped :)
Excluding it from the tarball seems like the best option?
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.
Oh, actually I did mean config.sample.json but I see what you mean: if you made a config.json before packaging, would you expect it to be in the package? Relatedly, I now can't easily un-tar a new version of vector onto my server and update the symlink as I have to copy the config.json over (unless I untar over the old directory but then I'll get stale files). I can't find anyone who's solved this nicely.
I guess excluding the config file from the tarball prevents us from accidentally shipping one we didn't intend to package, and doesn't really hurt. Probably best done in a different PR though rather than shoehorning it into here.
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.
agreed
To reflect the fact that you can now sensibly deploy from a package (ie. be able to configure the app). Make the first thing be downloading a prebuilt package so people who only read the first part don't end up running npm start in production and complain about needing npm.