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

services/horizon: Build and test verify-range Docker image #3575

Merged

Conversation

2opremio
Copy link
Contributor

Part of #2761

@2opremio 2opremio force-pushed the 2761-build-and-test-verify-range-image branch from 2f1a872 to f504c18 Compare April 29, 2021 11:21
@2opremio 2opremio requested a review from a team April 29, 2021 11:27
name: Build and test Verify Range Docker image
command: |
docker build -f services/horizon/docker/verify-range/Dockerfile -t stellar/horizon-verify-range services/horizon/docker/verify-range/
docker run -ti -e BRANCH=master -e FROM=15000063 -e TO=15000127 stellar/horizon-verify-range
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe there is a better range to test, but I didn't want it to take more than a few minutes.

@bartekn let me know if you have other range suggestions

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's other ranges to test, we could do a few different ranges in parallel on circle?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it would be nice to comment in the circle file why we chose these particular ranges.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to have a single range here. Isn't it added here just to check if the image works? I have some ideas when it comes to ranges but the image is run using the code from master so it wouldn't really test the PR.

name: Build and test Verify Range Docker image
command: |
docker build -f services/horizon/docker/verify-range/Dockerfile -t stellar/horizon-verify-range services/horizon/docker/verify-range/
docker run -ti -e BRANCH=master -e FROM=15000063 -e TO=15000127 stellar/horizon-verify-range
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to have a single range here. Isn't it added here just to check if the image works? I have some ideas when it comes to ranges but the image is run using the code from master so it wouldn't really test the PR.

@2opremio 2opremio force-pushed the 2761-build-and-test-verify-range-image branch from 74e973f to 005838d Compare May 4, 2021 17:46
@2opremio
Copy link
Contributor Author

2opremio commented May 4, 2021

Uhm, I rebased and the verify-range test gets stuck, I suspect it could be due to #3576

I will remove the PR and see if it passes

@2opremio 2opremio force-pushed the 2761-build-and-test-verify-range-image branch 2 times, most recently from 806f284 to 5702220 Compare May 4, 2021 21:19
@2opremio
Copy link
Contributor Author

2opremio commented May 5, 2021

I think it is due to #3576 (it works with BRANCH=05e09089828a0528e5545a04d7f713dee8f8e379). @paulbellamy could you take a look?

@2opremio 2opremio force-pushed the 2761-build-and-test-verify-range-image branch from 5b5ba31 to bcfbc7f Compare May 6, 2021 10:04
@2opremio
Copy link
Contributor Author

2opremio commented May 6, 2021

OK, it seems to randomly get stuck (it just worked with master).

@2opremio
Copy link
Contributor Author

2opremio commented May 6, 2021

I am thinking it may be a memory problem, so I am going to move to an even earlier range.

@2opremio 2opremio merged commit 15ccb7b into stellar:master May 6, 2021
@2opremio 2opremio deleted the 2761-build-and-test-verify-range-image branch May 6, 2021 12:09
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.

3 participants