Skip to content

Conversation

BlowaXD
Copy link
Contributor

@BlowaXD BlowaXD commented Jan 5, 2024

This should partially solve #45

Additions

  • added registry/Dockerfile
  • added registry/.dockerignore ignoring **/target/** to avoid having to sending too much files to context
  • added README.md in registry project with docker build command

I decided to manually install protobuff deps to have an easy way to manage the protobuf compiler version

@BlowaXD BlowaXD changed the title add dockerfile for registry Add Dockerfile for registry Jan 5, 2024
@tomkarw
Copy link
Contributor

tomkarw commented Jan 5, 2024

Shouldn't the Dockerfile live in /registry folder?

@mara-schulke
Copy link
Contributor

Hei @BlowaXD, tysm for your contribution! 😊

@mara-schulke mara-schulke self-assigned this Jan 5, 2024
@mara-schulke mara-schulke added component::registry Everything related to the buffrs registry priority::high Urgent change or idea, please review quickly integration Changes and ideas related to integrating buffrs with 3rd-party software or tools complexity::medium Issues or ideas which require some discussion, research and more implementation work labels Jan 5, 2024
@mara-schulke mara-schulke added this to the Registry milestone Jan 5, 2024
@mara-schulke
Copy link
Contributor

I agree with @tomkarw's comment, otherwise this looks good to me 😊

@BlowaXD
Copy link
Contributor Author

BlowaXD commented Jan 5, 2024

I agree with @tomkarw's comment, otherwise this looks good to me 😊

Sure, I did first assume that someone would have wanted to build from the same directory

Moving it to registry folder requires either:

Having this COPY statement instead of the current one (https://github.com/helsing-ai/buffrs/pull/206/files#diff-94b4fdf407a88fb5e59fcd22bd231c04f89deb5aa0bc67c37baedb600e2e6226R20)

COPY .. .

or using this command to build

docker build -t registry . --file registry/Dockerfile

I don't know which UX you want to provide, I do not see an alternative because of the registry requiring buffrs as a dependency

@tomkarw
Copy link
Contributor

tomkarw commented Jan 5, 2024

I don't know which UX you want to provide, I do not see an alternative because of the registry requiring buffrs as a dependency

I'd go with COPY .. ..

I think the issue is more general, we have a CLI, a registry, and they share some common parts. This clearly indicates 3 crates (cli, registry and buffrs-lib), but currently cli.contains(buffrs-lib). I'd move the Dockerfile and hack it to work, and we can open a ticket about at some point extracting the common parts out.

@tomkarw tomkarw self-requested a review January 5, 2024 16:15
@BlowaXD
Copy link
Contributor Author

BlowaXD commented Jan 6, 2024

Edited the PR, I gave a command in the registry/README to guide user to build the docker so it will be ready when registry works
As registry is a WIP, I didn't know what to write in the README so I decided to add a short disclaimer that can be removed whenever

- added README.md in registry project with docker build command
- added .dockerignore to ignore **/target/**
@mara-schulke mara-schulke merged commit c410657 into helsing-ai:main Jan 7, 2024
@mara-schulke
Copy link
Contributor

Awesome, thank you @BlowaXD!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity::medium Issues or ideas which require some discussion, research and more implementation work component::registry Everything related to the buffrs registry integration Changes and ideas related to integrating buffrs with 3rd-party software or tools priority::high Urgent change or idea, please review quickly
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants