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

Exposing a port in options. #8

Merged
merged 3 commits into from
Oct 9, 2022
Merged

Conversation

rdcm
Copy link
Contributor

@rdcm rdcm commented Oct 4, 2022

Hi!

Thanks for your project, very useful setting. Motivation:

  • the deterministic assigned port looks a little bit more predictable in our case
  • for our integration tests enough single MongoDB instance

@asimmon
Copy link
Owner

asimmon commented Oct 5, 2022

Hi @rdcm, thanks for your contribution, the changes looks fine to me! Two things though:

  • Could you also update the README so the MongoPort port property is shown in the example?
  • Also I recently introduced Microsoft.CodeAnalysis.PublicApiAnalyzers in the main branch to track public APIs changes. There should be a build warning because the MongoPort is not declared in PublicAPI.Shipped.txt. It can automatically be fixed with Visual Studio or Rider as there's a roslyn analyzer code fix.

Also seems like the CI action did not run on your branch, I'll try to understand why later.

@rdcm
Copy link
Contributor Author

rdcm commented Oct 5, 2022

But looks like local tests running requires environment setup.

@asimmon
Copy link
Owner

asimmon commented Oct 8, 2022

@rdcm as you can see the CI build is failing on your branch and it took me some time to understand the problem. It's not related to your modifications.

Basically the library keeps a reference to the options instance that are provided to MongoRunner.Run. So when these options are reused (such as in the tests) the properties have changed and we try to use the same port, same directories, etc. I fixed it in #9 by cloning the options.

During the investigation I found a way to optimize the replica set initialization #10, so thanks for this.

Now in order for me to merge your PR I need you to do two things:

  1. Merge main into your branch - which will fix the CI build
  2. Move the property MongoRunnerOptions.MongoPort above the line "Internal properties start here"

@rdcm rdcm force-pushed the expose_mongodb_port branch from 3b4f949 to 3b2a024 Compare October 9, 2022 08:19
@rdcm
Copy link
Contributor Author

rdcm commented Oct 9, 2022

@rdcm as you can see the CI build is failing on your branch and it took me some time to understand the problem. It's not related to your modifications.

Basically the library keeps a reference to the options instance that are provided to MongoRunner.Run. So when these options are reused (such as in the tests) the properties have changed and we try to use the same port, same directories, etc. I fixed it in #9 by cloning the options.

During the investigation I found a way to optimize the replica set initialization #10, so thanks for this.

Now in order for me to merge your PR I need you to do two things:

  1. Merge main into your branch - which will fix the CI build
  2. Move the property MongoRunnerOptions.MongoPort above the line "Internal properties start here"

Mission completed.

  • feature branch rebased on the main
  • MongoRunnerOptions.MongoPort moved to public section

@asimmon asimmon merged commit 311f786 into asimmon:main Oct 9, 2022
@asimmon
Copy link
Owner

asimmon commented Oct 11, 2022

These changes have been released on version 0.1.3 and are available on nuget.org.

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