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.

Sign up for GitHub

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

Jump to bottom

Add registry-watcher image and separate docker-compose services #2269

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add registry-watcher image and separate docker-compose services #2269

wants to merge 5 commits into from
+231 −157

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Oct 13, 2023

Creates a separate image for the registry-watcher, setup to run the correct command. Reconfigures the docker-compose setup to use the web-server image and this new registry-watcher image as intended to be used in the new production infrastructure, along with a new build-server image that is close to what will be used in the new production infrastructure, replicated twice.

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Oct 13, 2023
@Nemo157
Copy link
Member Author

Nemo157 commented Oct 13, 2023

Probably needs some infra input on the deploy changes (what exactly the tags they use will be). Also I want to investigate the crates.io-index initialization a little more and make sure that's working (and probably have it be local to each container instead of shared as it is now).

syphar
syphar requested changes Oct 14, 2023
View reviewed changes
dockerfiles/Dockerfile Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
dockerfiles/Dockerfile Outdated Show resolved Hide resolved
@syphar
Copy link
Member

syphar commented Oct 14, 2023

Thank you for working on this @Nemo157 !

Sorry for already adding a review in the draft-stage, but I thought some feedback would already help :)

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 14, 2023
@Nemo157 Nemo157 mentioned this pull request Oct 16, 2023
Merged
@Nemo157 Nemo157 mentioned this pull request Nov 15, 2023
Merged
@Nemo157 Nemo157 marked this pull request as ready for review November 15, 2023 15:53
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 15, 2023

I have removed the CI changes pushing the new image to production, since that will require some input from infra and can be done later.

@Nemo157 Nemo157 force-pushed the docker-compose-services branch 5 times, most recently from d7978cd to eb357e5 Compare November 16, 2023 09:38
@Nemo157 Nemo157 added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Nov 16, 2023
@Nemo157 Nemo157 requested a review from syphar November 16, 2023 09:56
syphar
syphar reviewed Nov 17, 2023
View reviewed changes
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

I wasn't able to fully test yet, but I already have some comments / ideas.

Also I want to investigate the crates.io-index initialization a little more and make sure that's working (and probably have it be local to each container instead of shared as it is now).

Were you able to test if the index clone works without the entrypoint.sh?

docker-compose.yml Outdated Show resolved Hide resolved
dockerfiles/Dockerfile Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
@syphar
Copy link
Member

syphar commented Nov 17, 2023 •
edited

short test result:

  • trying to run the registry watcher gives me /usr/bin/tini: invalid option -- '-'

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 18, 2023

Fixed that, I was normalizing the COMMANDs and thought that the -- wasn't actually needed with tini. I've normalized them the other way and split the ENTRYPOINT and COMMAND so that it's possible to run other cratesfyi commands under any image.

@Nemo157 Nemo157 mentioned this pull request Nov 18, 2023
Draft
@syphar
Copy link
Member

syphar commented Nov 19, 2023

Fixed that, I was normalizing the COMMANDs and thought that the -- wasn't actually needed with tini. I've normalized them the other way and split the ENTRYPOINT and COMMAND so that it's possible to run other cratesfyi commands under any image.

I'm still seeing the same error when trying to run the registry watcher container?

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Nov 22, 2023
@Nemo157 Nemo157 requested a review from a team as a code owner March 19, 2024 14:20
@github-actions github-actions bot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Mar 19, 2024
@Nemo157
Copy link
Member Author

Nemo157 commented Mar 19, 2024

I'm still seeing the same error when trying to run the registry watcher container?

Coming back to this finally, are you using --build with the docker compose up call? It doesn't have any kind of change detection and requires you to tell it to rebuild. I've added that to the comment about how to run it.

syphar
syphar requested changes Mar 20, 2024
View reviewed changes
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

tested

  • web container
  • cli container
  • registry watcher
  • builder-a

awesome, thank you for keeping to work on it!

Some small documentation comments, and a missing platform.

README.md
You can also run other non-build commands like the setup steps above, or queueing crates for the background builders from within the `cli` container:

```sh
docker compose run --rm cli database migrate
Copy link
Member

Choose a reason for hiding this comment

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

it's not clear that you have to run this command to make the service work.

docker-compose.yml
context: .
dockerfile: ./dockerfiles/Dockerfile
target: build-server
depends_on:
Copy link
Member

Choose a reason for hiding this comment

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

this is missing the platform: "linux/amd64" definition, or am I missing something?

README.md
a `registry-watcher` container:

```sh
docker compose run --rm registry-watcher queue set-last-seen-reference --head
Copy link
Member

Choose a reason for hiding this comment

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

same here, the command has to be run initially, to be able to use the registry-watcher container at all

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syphar syphar syphar requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Nemo157 @syphar