Skip to content

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Jan 16, 2023

Closes #133.

This needs to temporarily update all of our workflows and Docker image builds to ensure that they can reach the cometbft-db repository. I've logged an issue to keep track of reverting this in #158.

I'm currently manually running our nightly E2E tests on this branch to ensure that this still works: https://github.com/cometbft/cometbft/actions/runs/3952269253

This will have to be manually backported to v0.37.x and v0.34.x.

After this change, while the repos are still private, to be able to build the Docker images and run the E2E tests locally you'll need to do the following for each shell in which you're working:

export GOPRIVATE=github.com/cometbft
# This is needed because we need to grant access to the private cometbft-db repo 
# _within_ the Docker container build operation. Note the space before the "export"
# command - this prevents your shell from storing this in the history.
 export GO_MODULES_TOKEN="... a GitHub API token here..."

Your GitHub API token must have full access (read/write) to all repositories (i.e. the repo checkbox when configuring permissions). See https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/creating-a-personal-access-token

I'd recommend setting an expiry of 30 days to any tokens used for this purpose, so we don't accidentally leave them lingering.


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

@thanethomson thanethomson added the wip Work in progress label Jan 16, 2023
@thanethomson thanethomson changed the title Replace tm-db with cometbft-db Replace tm-db with cometbft-db on main Jan 17, 2023
@thanethomson thanethomson force-pushed the thane/133-cometbft-db branch from 8ab2565 to 625c509 Compare January 17, 2023 13:07
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
…uilding of database support

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson force-pushed the thane/133-cometbft-db branch from 134034e to c0b5511 Compare January 18, 2023 19:25
@thanethomson thanethomson marked this pull request as ready for review January 18, 2023 19:25
@thanethomson thanethomson requested a review from a team as a code owner January 18, 2023 19:25
@thanethomson thanethomson removed the wip Work in progress label Jan 18, 2023
Round: 1, Index: 1, Type: cmtproto.PrevoteType},
Message: &cmtcons.HasVote{
Height: 1,
Round: 1, Index: 1, Type: cmtproto.PrevoteType,
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] Might want to separate each field in its own line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's weird the formatter didn't pick this up. I suspect that eventually switching to something like gofumpt for formatting will help here, but we need to resolve some of our concerns around its use first.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
@lasarojc lasarojc added the rename Renaming our fork label Jan 19, 2023
@lasarojc
Copy link
Contributor

For me it worked only after i used export GOPRIVATE=github.com. I don't know if this is any particularity of my setup, but noting here in case anyone else also have problems.

@thanethomson thanethomson merged commit fbb9871 into main Jan 19, 2023
@thanethomson thanethomson deleted the thane/133-cometbft-db branch January 19, 2023 17:06
@thanethomson thanethomson mentioned this pull request Jan 19, 2023
3 tasks
mergify bot pushed a commit that referenced this pull request Jan 20, 2023
Follow-up to #137.

Fixes the Docker builds that were failing to be parsed by GitHub Actions.

Also fixes our `make build-docker` target to build images in the same way we do in CI. On Linux, I have to explicitly tell Docker to use BuildKit to pass the target/build platform args in:

```bash
DOCKER_BUILDKIT=1 make build-docker
```

Given that I've also proven now that the images do build in CI (see [this workflow](https://github.com/cometbft/cometbft/actions/runs/3964436268/jobs/6793294390) and [this one](https://github.com/cometbft/cometbft/actions/runs/3964436273/jobs/6793294238)), we no longer need to build on every change to every pull request. The E2E node Docker image in particular takes nearly 30mins to build, so that would slow us down tremendously. It's far better to build those images only once PRs get merged.

---

#### PR checklist

- [x] Tests written/updated, or no tests needed
- [x] `CHANGELOG_PENDING.md` updated, or no changelog entry needed
- [x] Updated relevant documentation (`docs/`) and code comments, or no
      documentation updates needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rename Renaming our fork
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

main: Replace tm-db with CometBFT DB
3 participants