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 containers #41

Merged
merged 17 commits into from
Feb 27, 2024
Merged

Update containers #41

merged 17 commits into from
Feb 27, 2024

Conversation

jbygdell
Copy link
Contributor

@jbygdell jbygdell commented Nov 2, 2023

**Note: this requires the deployment-fixes branch of GenomicDataInfrastructure/starter-kit-lsaai-mock unless that have been merged.

  • Updates the containers to be pulled from sensitive-data-archive
  • Updated the version of the MINIO container
  • Updates the configuration for the sda components
  • Updates the scripts for the demo setup
  • Updates the readme files
  • Adds an example with TLS configured

closes #40

@jbygdell jbygdell self-assigned this Nov 2, 2023
@jbygdell jbygdell force-pushed the update_containers branch 3 times, most recently from 5f83aa7 to 1695839 Compare November 16, 2023 07:10
Copy link
Contributor

@dbampalikis dbampalikis left a comment

Choose a reason for hiding this comment

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

Really nice in general, made some comments, mostly grammatical!

README.md Outdated Show resolved Hide resolved
config/TLS-example/README.md Outdated Show resolved Hide resolved
config/TLS-example/README.md Show resolved Hide resolved
config/TLS-example/README.md Outdated Show resolved Hide resolved
config/TLS-example/README.md Outdated Show resolved Hide resolved
config/TLS-example/README.md Outdated Show resolved Hide resolved
config/TLS-example/README.md Outdated Show resolved Hide resolved
config/TLS-example/README.md Outdated Show resolved Hide resolved
@jbygdell jbygdell force-pushed the update_containers branch 4 times, most recently from 34d6595 to 39d706b Compare November 16, 2023 08:30
@jbygdell jbygdell requested a review from dbampalikis November 17, 2023 12:32
dbampalikis
dbampalikis previously approved these changes Dec 4, 2023
docker-compose.yml Outdated Show resolved Hide resolved
servers/oidc.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nanjiangshu nanjiangshu 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! The docker setup runs well when I tested. I just left a few minor comments.

@dbampalikis dbampalikis self-requested a review December 12, 2023 10:19
Copy link
Contributor

@aaperis aaperis left a comment

Choose a reason for hiding this comment

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

Some changes break functionality, take a look at comments.

sda-admin needs to change as well to be compatible with the new images (this is just changing the default MQ_QUEUE_PREFIX to ``sdaon line 14 andaccessionIDs` to `accession` on line 505.

docker-compose.yml Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
servers/oidc.py Show resolved Hide resolved
config/TLS-example/config.yaml Outdated Show resolved Hide resolved
config/TLS-example/config.yaml Outdated Show resolved Hide resolved
.github/workflows/test_demo.yaml Outdated Show resolved Hide resolved
@jbygdell jbygdell force-pushed the update_containers branch 3 times, most recently from 128c8a3 to 334cf79 Compare December 15, 2023 14:37
Set correct queue/routingkey for accessions and fix general semantics.
@jbygdell jbygdell requested review from pahatz and aaperis February 15, 2024 17:47
@jbygdell jbygdell marked this pull request as ready for review February 15, 2024 17:49
@aaperis
Copy link
Contributor

aaperis commented Feb 16, 2024

Looks reasonable, but the new changes seem to have brought in a new error regarding scopes when auth talks to ls-aai-mock.

@jbygdell jbygdell force-pushed the update_containers branch 2 times, most recently from 1d44798 to 624d08e Compare February 16, 2024 17:13
@MalinAhlberg
Copy link
Contributor

I still can't get this to run. Even after docker system prune and following the README, download and s3inbox get the error:
Error while getting key http://aai-mock:8080/oidc/jwk: jwk.Fetch failed (failed to fetch \"http://aai-mock:8080/oidc/jwk\": Get \"http://aai-mock:8080/oidc/jwk\": dial tcp: lookup aai-mock on 127.0.0.11:53: server misbehaving) for http://aai-mock:8080/oidc/jwk","time":"2024-02-23T15:18:37Z"

while auth gets:
"oidc: issuer did not match the issuer returned by provider, expected \"http://dockerhost:8080/oidc/\" got \"http://localhost:8080/oidc/\"

Maybe we could try a pair debug next week.

Copy link
Contributor

@pahatz pahatz left a comment

Choose a reason for hiding this comment

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

Last changes did the trick, works great now.

Copy link
Contributor

@aaperis aaperis 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!
Some minor things need be added to the instructions for sda-admin usage.

quickstart-sda.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
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.

🥳

Co-authored-by: Alex Aperis <[email protected]>
@jbygdell jbygdell merged commit b395c0b into main Feb 27, 2024
1 check passed
@jbygdell jbygdell deleted the update_containers branch February 27, 2024 08:16
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.

Update image tags in the docker compose files
6 participants