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

Docs: How to run SDA stack #1162

Merged
merged 10 commits into from
Dec 10, 2024
Merged

Docs: How to run SDA stack #1162

merged 10 commits into from
Dec 10, 2024

Conversation

nanjiangshu
Copy link
Contributor

Related issue(s) and PR(s)
This PR closes #1153

Changes made

  • Fixed some bugs in the Makefile
  • Moved the instructions to run the SDA stack from GETTINGSTARTED.md to README.md and updated the instructions.

Copy link
Collaborator

@jbygdell jbygdell left a comment

Choose a reason for hiding this comment

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

Commit 282896d can be removed in it's entirety since the --remove-orphans in the compose down command will do just that.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@nanjiangshu
Copy link
Contributor Author

Commit 282896d can be removed in it's entirety since the --remove-orphans in the compose down command will do just that.

The problem is on Linux Ubuntu, the exited container for the integration test is not removed by --remove-orphans and that prohibit the removal of volumes.

@jbygdell
Copy link
Collaborator

jbygdell commented Dec 5, 2024

Commit 282896d can be removed in it's entirety since the --remove-orphans in the compose down command will do just that.

The problem is on Linux Ubuntu, the exited container for the integration test is not removed by --remove-orphans and that prohibit the removal of volumes.

How come then that I haven't seen this issue on Ubuntu for the past several years.

@nanjiangshu
Copy link
Contributor Author

Commit 282896d can be removed in it's entirety since the --remove-orphans in the compose down command will do just that.

The problem is on Linux Ubuntu, the exited container for the integration test is not removed by --remove-orphans and that prohibit the removal of volumes.

How come then that I haven't seen this issue on Ubuntu for the past several years.

No idea. I have problems on two of the VMs with Ubuntu 22.04, but another VM with Ubuntu 18.04 actually works. I will remove the commit but keep it only locally.

@nanjiangshu nanjiangshu force-pushed the docs/run-docker-setup branch from 05953bf to 473be23 Compare December 5, 2024 12:22
Copy link
Contributor

@MalinAhlberg MalinAhlberg left a comment

Choose a reason for hiding this comment

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

Good work to document all this @nanjiangshu 👏 !
I have added some comments/suggestions. I also wonder if it would make more sense to keep most of this in the GETTINGSTARTED and link it from the README? Since it's so detailed (which is very good but maybe a bit too much for the readme). And in that case also update the help text of the Makefile.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@nanjiangshu
Copy link
Contributor Author

nanjiangshu commented Dec 7, 2024

Good work to document all this @nanjiangshu 👏 ! I have added some comments/suggestions. I also wonder if it would make more sense to keep most of this in the GETTINGSTARTED and link it from the README? Since it's so detailed (which is very good but maybe a bit too much for the readme). And in that case also update the help text of the Makefile.

That's a good point. However, as I discussed with @jbygdell and @viklund in one of the standups, we believe that a shorter README might lead people to overlook these instructions. Currently, the text isn't too long, but I agree that if it grows in the future, we should consider making it more structured and moving the detailed instructions elsewhere. We can probably create a refactoring issue for the development instructions and move some of the instructions to https://github.com/neicnordic/sensitive-data-archive/blob/feature/manual-go-run/DEVELOPMENT.md

MalinAhlberg
MalinAhlberg previously approved these changes Dec 9, 2024
jbygdell
jbygdell previously approved these changes Dec 9, 2024
@nanjiangshu nanjiangshu dismissed stale reviews from jbygdell and MalinAhlberg via 441cc5f December 9, 2024 14:55
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@nanjiangshu nanjiangshu force-pushed the docs/run-docker-setup branch 3 times, most recently from 30c7ed1 to 96adbd3 Compare December 9, 2024 18:45
MalinAhlberg
MalinAhlberg previously approved these changes Dec 10, 2024
@nanjiangshu nanjiangshu changed the base branch from main to feature/manual-go-run December 10, 2024 08:59
@nanjiangshu nanjiangshu changed the base branch from feature/manual-go-run to main December 10, 2024 09:00
@nanjiangshu nanjiangshu dismissed MalinAhlberg’s stale review December 10, 2024 09:00

The base branch was changed.

@nanjiangshu nanjiangshu force-pushed the docs/run-docker-setup branch from e39e066 to 30c7ed1 Compare December 10, 2024 09:06
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@nanjiangshu nanjiangshu force-pushed the docs/run-docker-setup branch from 8deb24e to 06d83ad Compare December 10, 2024 10:04
README.md Outdated Show resolved Hide resolved
jbygdell
jbygdell previously approved these changes Dec 10, 2024
Copy link
Collaborator

@jbygdell jbygdell left a comment

Choose a reason for hiding this comment

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

Just one tiny thing.

Co-authored-by: Joakim Bygdell <[email protected]>
@nanjiangshu
Copy link
Contributor Author

Hi @jbygdell and @MalinAhlberg , committed some tiny formatting changes to the docs and the PR needs your re-approval. Thank you.

@nanjiangshu nanjiangshu added this pull request to the merge queue Dec 10, 2024
Merged via the queue into main with commit e6e05ab Dec 10, 2024
7 checks passed
@nanjiangshu nanjiangshu deleted the docs/run-docker-setup branch December 10, 2024 13:41
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.

Documentation for running docker compose
3 participants