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 setup_script to fetch landing page #101

Merged
merged 6 commits into from
May 7, 2021
Merged

Update setup_script to fetch landing page #101

merged 6 commits into from
May 7, 2021

Conversation

siddhanth339
Copy link
Contributor

Summary

The setup_script was updated to include all the required files to run the application smoothly using node.

Motivation

Testing

It has been tested manually on my local system

@birm birm self-requested a review May 3, 2021 22:03
Copy link
Member

@birm birm 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 opening this! A good start, but I think this can go in one of two different ways. Whichever way you find more useful, make sure to document :)

setup_script.sh Outdated
###
echo "Checking for required files"

if [[ ! -d ../Distro/db ]]
Copy link
Member

Choose a reason for hiding this comment

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

For this, it should be sufficient to check that whichever mongo uri is being used is accessible. Distro/db is just a hacky way to ensure persistence/fs backups of the mongo data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the lines as there is a check for mongo uri in the setup script and committed the changes: 05bd6cc

setup_script.sh Show resolved Hide resolved
@siddhanth339 siddhanth339 requested a review from birm May 4, 2021 04:39
@birm
Copy link
Member

birm commented May 6, 2021

Since backup-dev is currently holding @YashKumarVerma 's work, I'd appreciate their input here too.

@YashKumarVerma
Copy link
Contributor

Sure, I'll go through this

@siddhanth339
Copy link
Contributor Author

Thanks for your time!

Copy link
Contributor

@YashKumarVerma YashKumarVerma left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks !

Comment on lines 46 to 49
// connector.init().catch((e) => {
// console.error("error connecting to database");
// process.exit(1);
// });
Copy link
Contributor

@YashKumarVerma YashKumarVerma May 7, 2021

Choose a reason for hiding this comment

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

I do not think these lines are needed anymore as this is being done in caracal.js

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, you are right! They have been commented out.

@YashKumarVerma
Copy link
Contributor

YashKumarVerma commented May 7, 2021

A pull request (#98) was submitted resolving the issue (#96) before this pull request was sent.

I'm a bit confused on this part:

@birm

Thanks !

@birm
Copy link
Member

birm commented May 7, 2021

Ah, this must be a peril of messing with commit history. None of the commits in this PR changed matching #98, but the changes are part of this anyway. I think it's resolved.

I'll do a fair bit more testing before merging backup-dev in, but I think this is good to go. Any objections?

@siddhanth339
Copy link
Contributor Author

None from my side!

@birm birm merged commit 1d7e113 into camicroscope:backup-dev May 7, 2021
@siddhanth339
Copy link
Contributor Author

Thanks!!

@birm birm mentioned this pull request Jun 16, 2021
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