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

Updated/created script for handling prerequisites #268

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

axelbjo
Copy link
Contributor

@axelbjo axelbjo commented Aug 26, 2024

No description provided.

package.json Outdated
@@ -26,6 +26,7 @@
"@kth/server": "^4.1.0",
"body-parser": "^1.20.2",
"cookie-parser": "^1.4.6",
"csv-parser": "^3.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be check-in as its not used by the application itself. We just need to install in locally we run the import script.

Copy link
Contributor

@karlandindrakryggen karlandindrakryggen left a comment

Choose a reason for hiding this comment

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

Update the README with a section for the new script, And I think we should same pattern as the old scripts with a .sh and --env for loading env variables.

@@ -1,5 +1,8 @@
const log = require('@kth/log')
const nodeMongo = require('@kth/mongo')
const dotenv = require('dotenv')
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this was part of the confusion when I tried it as well as this wasn't part of the other PR. In the script Benni created he used the --env param to node to load a specific env-fil, and --env needs node 20.

I like Benni approach with a separate .env-file and use of --env since it will be super clear which env-file that is used, and it will be less likely that you make a misstake to use wrong mongo db instance.

What do you think? We can discuss this further tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, now I think I remember that the reason for why I added this was because I got the connection string error as well when I tried to connect to my local db, but I think that Benni's approach sounds better! I'll be happy to discuss it tomorrow to understand it a bit better as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to add a *.sh script similar to scripts/importSupplementaryInfo.sh and add a section about that in the README (scripts/importData/README.md) as well, so that it is the same as the old scripts.

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.

2 participants