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

Enable custom port and user for mongodb + fixed typo in readme #9

Merged
merged 5 commits into from
Aug 25, 2023

Conversation

jesper-friis
Copy link
Contributor

No description provided.

Copy link
Collaborator

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

While the affected environment variables in the docker compose file can be set explicitly and individually when running docker compose - that's the point of them, I suppose having a single environment variable that affects both would be fine in this case.

Please revert your current changes to the README.md, but instead perhaps add information about your current addition to the docker-compose.yml file?

README.md Outdated
Comment on lines 52 to 53
docker-compose pull
docker-compose up --build
Copy link
Collaborator

Choose a reason for hiding this comment

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

docker compose is the correct way to do this:
image

This image is taken from the official Docker documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Then the corresponding documentation for oteapi services should be updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I referenced this comment in the OTEAPI Service issue here.

README.md Outdated
@@ -95,5 +95,5 @@ For using it with Docker, use the `--env-file .env` argument when calling `docke

## Licensing & copyright

All files in this repository are [MIT licensed](LICENSE).
All files in this repository are [MIT licensed](LICENSE).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to have a new line, but not new section here. Hence the end-of-line double spaces, which does exactly this.

Copy link
Contributor Author

@jesper-friis jesper-friis May 12, 2023

Choose a reason for hiding this comment

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

OK. Such blanks at end of line are stripped off by default by many editors...

@CasperWA
Copy link
Collaborator

While you re-requested a review, no updates have been pushed from your side to fix the requested changes, so my current review still stands.

Copy link
Collaborator

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

The previous review still stands.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Revert changes to README.md
@CasperWA CasperWA enabled auto-merge (squash) August 25, 2023 08:09
@CasperWA CasperWA disabled auto-merge August 25, 2023 08:11
@CasperWA CasperWA merged commit 90987c5 into SINTEF:main Aug 25, 2023
@CasperWA
Copy link
Collaborator

Force-merging this with changes to README reverted. The reasons for the reversions have been given above in the reviews done in spring, but no reaction has occurred on the side of the PR responsible.
If changes are still desired to the README, please open an issue to explain what changes should be done and why.

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