Docker-targets v2 #10346
Docker-targets v2 #10346
Conversation
…cker-targets"" This reverts commit d972c46. It also fixes up some conflicts / inconsistencies, since op-conductor was added after the original revert.
bitwiseguy
force-pushed
the
ss/docker-targets-v2
branch
from
74ea1a6
to
9ff0769
Compare
bitwiseguy
requested review from
a team,
0x00101010,
zhwrd and
mslipper
as code owners
WalkthroughWalkthroughThe modifications across various configuration files and Makefiles primarily focus on standardizing the versioning system ( Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (5)
docker-bake.hcl (1)
[!TIP]
Codebase VerificationThe verification process has confirmed that most of the new variables introduced in the
docker-bake.hclfile are correctly used in the Docker build targets. However, there are exceptions:
IMAGE_TAGSis not used in the targets.PLATFORMSis not used in the targets.These findings suggest that while the majority of the variables are implemented correctly, there are issues with how
IMAGE_TAGSandPLATFORMSare utilized, or potentially not utilized, within the build targets. This could lead to unexpected behavior or errors during the Docker image build process if these variables are intended to be used but are not correctly referenced.Action Items:
- Review the intended use of
IMAGE_TAGSandPLATFORMSto ensure they are either removed if unnecessary or correctly implemented in the Docker build targets.Analysis chain
Line range hint
17-212: Verify that all new variables introduced indocker-bake.hclare correctly used in the Docker build targets and that their default values are appropriate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new variables in Docker build targets. # Test: Check each target in `docker-bake.hcl` for correct usage of variables. echo "Check each Docker build target for correct variable usage."Length of output: 126
Script:
#!/bin/bash # Description: Verify the usage of new variables in Docker build targets by checking their presence in the target definitions. # Extracting the variable names and checking their usage in the target definitions. # List all variable names variable_names=$(grep -oP 'variable "\K[^"]+' docker-bake.hcl) # Check each variable usage in the target definitions for var in $variable_names; do echo "Checking usage of variable: $var" grep -q "\${$var}" docker-bake.hcl && echo "$var is used in the targets." || echo "$var is NOT used in the targets." doneLength of output: 3304
.circleci/config.yml (4)
1838-1838: Clarify the comment regarding the non-inclusion ofop-programin the devnet.Consider enhancing the comment to explain why
op-programis not part of the devnet, as this could be useful for future maintainers.
1848-1848: Clarify the comment regarding the non-inclusion ofop-challengerin the devnet.Consider enhancing the comment to explain why
op-challengeris not part of the devnet, as this could be useful for future maintainers.
1858-1858: Clarify the comment regarding the non-inclusion ofop-conductorin the devnet.Consider enhancing the comment to explain why
op-conductoris not part of the devnet, as this could be useful for future maintainers.
1863-1863: Clarify the comment regarding the non-inclusion ofop-heartbeatin the devnet.Consider enhancing the comment to explain why
op-heartbeatis not part of the devnet, as this could be useful for future maintainers.
We have been thinking about removing the devnet from the monorepo and finding an owner for it as right now nobody truly owns the code |
With docker-layer-caching and the use of the go-mod and go-build cache subsequent runs should be faster. On a fresh run it may take longer. |
|
@protolambda @tynes fixed the issue of slow build time during The build time was slow because I wasn't using the |
|
@protolambda would it be worth me spending time to setup an automated Circle CI test to check that the docker images run on x86 machines? Or is that something you want to keep as a manual check? |
|
@bitwiseguy nice work on getting the docker change back into shape!
You mean Arm machines? CI already runs in x86 I believe. Most of the dev machines run apple m1/m2, with a few exceptions. I think it makes sense to test ARM (circle CI should have an option to select the type of machine), but just a sanity check would be enough, like running with the |
|
Semgrep found 3
Named return arguments to functions must be appended with an underscore ( |
bitwiseguy
force-pushed
the
ss/docker-targets-v2
branch
from
a07a168
to
cb73253
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Reviving #8721. This PR was generated by:
docker-targets-v2calledss/docker-targets-v2developbranchop-dispute-mon,da-server) introduced sincedocker-targets-v2was createdThis PR accomplishes the following:
🟢 Adds a script to Circle CI
config.ymlto pass git tags todocker buildxso that docker containers report the appropriate git tag version of code that is used to build the image. Previously versions were always incorrectly reported asv0.0.0🟢 Reduces the average duration of the
op-*-docker-buildCircle CI jobs reduced from ~4m45s --> ~45s (due to new docker build flow)🟢 Adds a Circle CI job called
check-cross-platformto ensure images can be run on ARM machines (even though the docker build+push is performed on an AMD machine)Tests
Manually tested the new versioning script at the following commits to ensure the version was set correctly in each case:
caf41c524a8d3e06e61c7a8938dfb7a591345a437036381b036ae364125169e8b1d7915aecdcbac859cfdbeTested the new Circle CI job
check-cross-platformby explicitly passing it the docker image tage87e5ef2b96893eb8b446da420f7ba7f3e3c5985forop-node. When this job is actually triggered (after docker-build is run with thepublishflag set), it should use the git commit of the current branch as the docker image tag to pull from the docker registry.Locally ran the Circle CI
check-cross-platformscript against all op components (op-node,op-proposer, etc) that now have thecheck-cross-platformjob applied to them in the Circle CIconfig.yml. Confirmed that each one runs successfully when given a docker tag that exists in the docker registry.Metadata