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

setup docker build in cicd #1

Merged
merged 2 commits into from
Apr 18, 2023
Merged

setup docker build in cicd #1

merged 2 commits into from
Apr 18, 2023

Conversation

geekflyer
Copy link
Contributor

No description provided.

@geekflyer geekflyer force-pushed the docker-cicd branch 18 times, most recently from 94f3e61 to ebc63f0 Compare April 18, 2023 06:14
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we usually commit the .vscode folder? Also I was looking for a python auto-formatter but none of them worked, I'll try the ones you've listed in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have preference to format as github action (internal ops uses super-linter) https://github.com/aptos-labs/internal-ops/pull/1102/

I'm also not sure we need to commit .vscode unless we have an opinionated way for others to work using this ide

Copy link
Contributor Author

@geekflyer geekflyer Apr 18, 2023

Choose a reason for hiding this comment

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

I think the general practice is to only check in select files of the .vscode folder that are useful to repro a specific vs code setup across the team. In this case I'm just checking in the workspace file which sets up the workspace, but otherwise isn't too opinionated. You don't have to open the workspace file btw and then this file is practically being ignored, but I highly recommend it as this will make sure the code completion on python projects works correctly, incl. all the thirdparty dependencies.
Just as an example vscodes github repo itself has checked in some .vscode settings as well: https://github.com/microsoft/vscode/tree/main/.vscode

Copy link
Contributor Author

@geekflyer geekflyer Apr 18, 2023

Choose a reason for hiding this comment

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

I would have preference to format as github action (internal ops uses super-linter) https://github.com/aptos-labs/internal-ops/pull/1102

Agree. We can introduce sth like that in another PR. super-linter would also just invoke black so this wouldn't really change the VS code settings per-se.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this action work for Typescript and Rust builds too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but you have to add typescript/rust etc. to the matrix configuration once we're ready to build these.

@@ -4,16 +4,15 @@ FROM python:3.11
RUN pip install "poetry==1.4.2"

Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth switching over to python-slim rather than regular python image? will this be widely distributed?

Copy link
Contributor Author

@geekflyer geekflyer Apr 18, 2023

Choose a reason for hiding this comment

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

hmm. The dockerhub python guide kinda "highly" recommends to use the default image https://hub.docker.com/_/python unless there are specific circumstances like space constraints etc.
I think this image will be only distributed internally as-is (since this is just building the example code) so for now I wouldn't worry about this too much.

I think if in future we distribute something like this as base image we can revisit this.

@larry-aptos
Copy link
Collaborator

larry-aptos commented Apr 18, 2023

also a question to this repo: for community, what's their go-to place for generated code?

  1. this repo since we included here
    a. but i would prefer we should have a general repo name like indexer-grpc-ecosystem or indexer-grpc.
  2. or is our recommendation is to leave this to community to compile? if we want them to compile, how to manage the protobuf release?
  3. or we will create a new repo

@geekflyer
Copy link
Contributor Author

geekflyer commented Apr 18, 2023

also a question to this repo: for community, what's their go-to place for generated code?

  1. this repo since we included here
    a. but i would prefer we should have a general repo name like indexer-grpc-ecosystem or indexer-grpc.
  2. or is our recommendation is to leave this to community to compile? if we want them to compile, how to manage the protobuf release?
  3. or we will create a new repo

I think ideally we should actually publish the generated code as client libraries onto the respective registries, such as npm and pypi. Generating and publishing these SDKs can be probably done in aptos-core. cc @rtso

@geekflyer geekflyer merged commit 0c22d9d into main Apr 18, 2023
@geekflyer geekflyer deleted the docker-cicd branch April 18, 2023 17:23
@rtso
Copy link
Collaborator

rtso commented Apr 18, 2023

also a question to this repo: for community, what's their go-to place for generated code?

  1. this repo since we included here
    a. but i would prefer we should have a general repo name like indexer-grpc-ecosystem or indexer-grpc.
  2. or is our recommendation is to leave this to community to compile? if we want them to compile, how to manage the protobuf release?
  3. or we will create a new repo

I think ideally we should actually publish the generated code as client libraries onto the respective registries, such as npm and pypi. Generating and publishing these SDKs can be probably done in aptos-core. cc @rtso

That's a good idea and would make dependency management easier too as we continue to upgrade our indexer. We also will need to decide if this should belong in the existing aptos SDK or a new SDK specifically for indexing, which makes more sense once the indexer SDK provides more functionality.

For now when the protobuf gets updated (which should be infrequent), I was thinking we can generate the code and update this repo. I can publish the protobuf commands to do that in aptos-core.

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.

4 participants