-
-
Notifications
You must be signed in to change notification settings - Fork 995
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
Mongodb setup bug + tests #2257
Mongodb setup bug + tests #2257
Conversation
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Oops, made the PR on |
@pranshugupta54 Please fix the failed tests. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2257 +/- ##
===========================================
+ Coverage 98.45% 98.52% +0.06%
===========================================
Files 349 349
Lines 25168 25173 +5
Branches 2347 2351 +4
===========================================
+ Hits 24779 24801 +22
+ Misses 386 369 -17
Partials 3 3 ☔ View full report in Codecov by Sentry. |
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.
See comments
@@ -71,7 +64,7 @@ export async function askForMongoDBUrl(): Promise<string> { | |||
type: "input", | |||
name: "url", | |||
message: "Enter your MongoDB URL:", | |||
default: process.env.MONGO_DB_URL, | |||
default: "mongodb://localhost:27017", |
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.
- What happens if the parameter is already defined and it's not
localhost
or port27017
? Will the application break? - Can a ternary be used?
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.
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.
but if another url is set in our env, it won't show that, it'll always show "mongodb://localhost:27017"
the line should be this default: process.env.MONGO_DB_URL ?? "mongodb://localhost:27017"
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.
check the screenshot in previous comment, it's showing different. The port is different from default.
default means - when we have Yes/No type, do you see one of them marked automatically and u press enter?
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.
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.
You said it doesn't show from env. That url is from env itself, check the port. It's different port from default
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.
what? we're talking about the line that Peter commented on, you've hard coded mongodb://localhost:27017
there, so when that function runs, it'll always show mongodb://localhost:27017
for that prompt.
You said it doesn't show from env. That url is from env itself, check the port. It's different port from default
the screenshot you've provided is just for a log link, it will show what's in the .env
. The prompt you've modified is different, with its default value hard-coded, that's what we mean here.
Thanks,
Setup is the first DevOps and developer experience with the code. It needs to be perfect. |
@palisadoes Steps:
What I've done now,
We also kinda discussed a bit about this on Slack before this PR: https://thepalisadoes-dyb6419.slack.com/archives/CSWH4PN0M/p1713863659923539 |
This is bad, things are messy here, the way the setup for mongoDB is written is very ambiguous and confusing. The purpose of These are the things that need attention in the current implementation:
So, the comments and function/variable names need to be modified, and more/proper checks need to be added. |
I agree with the bad naming of variables and comments. I'll probably fix the naming and comments in this PR itself today.
@meetulr, we are actually not checking this for any step. If variable is found then we skip for every step like Redis etc. |
@meetulr, other than naming and comments - do you find any issue with the fixed implementation? Or PR looks better than current situation |
The purpose is to setup up a running url right, I'm saying that if a url is found in
|
We're not checking if it's already in env, it will directly be seen when we start the server. There's no point checking for previous values in setup. We don't run setup everytime we start the server so it's just for setting up mostly on the first time. Will fix the 2nd point, Yes! |
This is confusing:
Previous values? setup is meant to run first, and check if things are ready to run. It can be run again, and do the same thing.
exactly, setting up is meant to set things up so that the app can run properly. |
look what you are saying, makes complete sense that we should check for whether a connection can be established, even if the user opts not to change the URL. It definitely won’t take a long time, so this is a valid point. But you people must also realise that this setup was written. I’m not sure maybe more than a year ago. In fact when I worked on the set up a few months ago, my job was to refactor it and write tests. And yes, comments and the variable names are definitely ambiguous sometimes. But again, these were written by someone maybe more than a year ago, so we don’t know what was going on in his mind and maybe these were written by multiple people. The repository was quite different back then. So all we can do is if we find any errors, then we should discuss like @pranshugupta54 took initiative, and fix them. |
I don’t think this is confusing, look the purpose of the set up was to ensure that while you are initially setting up the application, all the variables and parameters provided are working properly in order for the application to start up. |
Ok understood. Thanks👍 |
Brother, the purpose of my comments is not to blame anyone😂, but to point out the things that can be / need to be improved😅.
Exactly, thus my insistence on comprehensive checks.
I'm saying that if there exist a url and user doesn't change it we're not checking if there's running instance there. That contradicts your first point which is what I was saying. Assuming part is my concern, when user changes the url, we "assume" that we need to check for a running instance, when they don't, we "assume" that it running. "again"? we're checking for a url for a running instance right?
Again, you're looking at it from the perspective of defending the code😂 which no one is blaming you for. It is Ofcourse, It is not a completely ureasonable assumption, but it is that, an "assumption". We could go with the current implementation assuming that. I Just thought it wouldn't be much work to just check it always. The main reason why I got confused was the variable/function naming and comments. This assumption on checks was another thing that came to attention, so I wanted to discuss it here.😅 See this |
@pranshugupta54 The default is not just used for when there's no url, it would be promped even when there's no instance running on a provided url, for cases where a user forgot to run the instance before starting the setup script, then, the user would be promped with their provided url (if not hard-coded like it is here), they can simply start the instance and continue the setup process. This could be a very common situation right? This is why I'm saying it should be this |
It's not really important to check if instance is running or not, this is just setup!! Instance may or may not be running when user runs the actual server 😂 So it's like we're checking running instance today but user starts development mode tomorrow, so no point checking running instances while setup |
Now this is just flipping the whole thing😂. It is for checking a running instance, try running it without a running instance (when you opt to change the url), you would be stuck in a loop.
We're checking for running instances, if not, we wouldn't be able to proceed. this is what the check is doing link
That's what I assumed until today, before I looked at the code, it is checking for running a instance. See this |
These were the confusions😂 maybe a clear description is need as to what it is meant to do. Is it this: That we're just checking whether the user is able to establish a connection with the mongo url they provide, regardless of when they actually mean to run the app, right after, or tomorrow. |
I've made a flow on how everything will work after this PR is merged. We do NOT need The following is how setup will work: Apart from this, we need to add some more comments for each function. Everything in this PR looks fine except some function comments. |
This seems to be a valid point. Suggesting new developers and first time comers the exact value of the URL, which is probably used 90% of times in localhost, will aid them in the setup as many people use mongoDB for the first time. The flowchart is also correct. The PR looks good. But one question, why is the event field changed in the schema in this pull request? Is that relevant to this issue? |
It is the recent work of @meetulr to remove event status. I think it added into this PR of pulling with latest upstream in Pranshu local branch. |
It got automatically added by husky - pre-commit -> |
Yeah sorry about that😅, schema file didn't get updated in my pr because i was having weird local issues with lint-staged, so i made a commit with --no-verify, and forgot to add schema file later😅 |
And one thing I like to add, A long back we had a PR merged to check the replicaSet or not and we have standardised it for a reason and me, @meetulr and @palisadoes had reviewed them. But it didn't introduced a way to use it, except checking for replicaSet or not. Why don't we standardise this check strictly in setup to connect for a replicaSet? |
What's the status on this? It's not clear whether there approval for this PR or not |
@palisadoes, the flow is fine. I've added comments for functions which looked unclear. Check #2257 (comment) |
@varshith257 @meetulr @Anubhav-2003 PTAL once more |
@pranshugupta54 Everything seems fine but what about this? |
I still have my doubts about variable naming😅. But others agree that these changes are good. We can see and change them later if people get confused. |
@varshith257 do you mean to make replica set mandatory? Is that an appealing solution🤔
The way that check worked is that if we connect to a replica set, session would be initiated and we'll have transactions support, if not, session would not be initiated and the functionalities would still work. If making replica set mandatory is what's required, it should be its own separate issue? |
I agree on this point. This could be handled in a separate issue which I think @varshith257 you can create. As for this PR. Everything seems fine including the flowchart. |
If you have some doubts then you can suggest them in the review here. Then we can workout something all together. |
Most of the setup file is using similar variable name, so changing one of mongodb variable would require to change redis and more too. So I've simply added comments in a better way to make them easy. We can take replicaSet in a separate issue, I'm not very sure about how it works. |
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.
LGTM
Before merging. Will this allow setup to work with a local Mongo installation either:
We had problems with this before. We need functionality in all scenarios. |
It's functionality is without docker |
85c2684
into
PalisadoesFoundation:develop
What kind of change does this PR introduce?
Bug fix + added tests
Issue Number:
Fixes #2255
Did you add tests for your changes?
yes
Snapshots/Videos:
If relevant, did you update the documentation?
NA
Summary
env
causing no value in the final env.Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?