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

Migrate CI to use coreum-builder #798

Merged
merged 29 commits into from
Apr 16, 2024
Merged

Migrate CI to use coreum-builder #798

merged 29 commits into from
Apr 16, 2024

Conversation

wojtek-coreum
Copy link
Collaborator

@wojtek-coreum wojtek-coreum commented Apr 2, 2024

Description

  • download go dependencies
  • revert to building required components only
  • support --help command for builder

Reviewers checklist:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

@wojtek-coreum wojtek-coreum requested a review from a team as a code owner April 2, 2024 07:32
@wojtek-coreum wojtek-coreum requested review from masihyeganeh, dzmitryhil, miladz68 and ysv and removed request for a team April 2, 2024 07:32
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.89%. Comparing base (3009107) to head (6642185).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #798      +/-   ##
==========================================
+ Coverage   34.69%   35.89%   +1.20%     
==========================================
  Files         171      171              
  Lines       48334    48334              
==========================================
+ Hits        16769    17351     +582     
+ Misses      28316    27712     -604     
- Partials     3249     3271      +22     
Flag Coverage Δ
coreum 31.49% <ø> (ø)
coreum-integration-tests-modules 22.22% <ø> (+5.68%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ysv
ysv previously approved these changes Apr 2, 2024
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @masihyeganeh, and @miladz68)

ysv
ysv previously approved these changes Apr 2, 2024
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @masihyeganeh, and @miladz68)

dzmitryhil
dzmitryhil previously approved these changes Apr 3, 2024
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh and @miladz68)

# Conflicts:
#	.github/workflows/ci.yml
@wojtek-coreum wojtek-coreum dismissed stale reviews from dzmitryhil and ysv via 580f4ad April 4, 2024 07:57
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh and @wojtek-coreum)


.github/workflows/ci.yml line 36 at r4 (raw file):

          - ci_step: "integration tests coreum-modules"
            command: |
              coreum-builder build/integration-tests/modules build images

It was decided to merge all build related steps into a separate step. You should merge all these build commands there

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 8 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @ysv)


.github/workflows/ci.yml line 36 at r4 (raw file):

Previously, miladz68 (milad) wrote…

It was decided to merge all build related steps into a separate step. You should merge all these build commands there

Now, dependencies are downloaded before building, so cache will be fine

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh and @wojtek-coreum)


.github/workflows/ci.yml line 36 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Now, dependencies are downloaded before building, so cache will be fine

was this discussed in the team ?

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh and @miladz68)


.github/workflows/ci.yml line 36 at r4 (raw file):

Previously, miladz68 (milad) wrote…

was this discussed in the team ?

yes, while you was on vacation. We may do it again if you want to

miladz68
miladz68 previously approved these changes Apr 9, 2024
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)


.github/workflows/ci.yml line 36 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

yes, while you was on vacation. We may do it again if you want to

No need. Approved.

@ysv ysv changed the title CI changes Migrate CI to use coreum-builder Apr 12, 2024
dzmitryhil
dzmitryhil previously approved these changes Apr 12, 2024
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)

keyleu
keyleu previously approved these changes Apr 12, 2024
Copy link
Collaborator

@keyleu keyleu left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r1, 7 of 7 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)

@ysv ysv dismissed stale reviews from keyleu and dzmitryhil via c4a1c5c April 12, 2024 16:57
@ysv ysv requested review from dzmitryhil and keyleu April 15, 2024 10:28
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 9 files reviewed, all discussions resolved (waiting on @dzmitryhil, @keyleu, and @masihyeganeh)


integration-tests/modules/staking_test.go line 292 at r9 (raw file):

// delegation is higher than current validator self delegation.
func TestValidatorUpdateWithLowMinSelfDelegation(t *testing.T) {
	// Since this test changes global staking config we can't run it in parallel with other tests.

evidence of failure: https://github.com/CoreumFoundation/coreum/actions/runs/8680049544/job/23819785222?pr=798#step:15:1510

dzmitryhil
dzmitryhil previously approved these changes Apr 15, 2024
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 9 files reviewed, all discussions resolved (waiting on @keyleu and @masihyeganeh)

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 7 files at r8, 1 of 2 files at r9, 5 of 5 files at r12.
Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @keyleu and @masihyeganeh)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 10 of 12 files reviewed, all discussions resolved (waiting on @keyleu, @masihyeganeh, and @ysv)

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 7 files at r8, 1 of 2 files at r9, 3 of 5 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @keyleu and @masihyeganeh)

@ysv ysv merged commit 3c5ad4a into master Apr 16, 2024
10 checks passed
@ysv ysv deleted the wojciech/ci branch April 16, 2024 07:42
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.

5 participants