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

Update CI with Github actions artifacts #6877

Merged
merged 1 commit into from May 3, 2024
Merged

Update CI with Github actions artifacts #6877

merged 1 commit into from May 3, 2024

Conversation

y4ssi
Copy link
Collaborator

@y4ssi y4ssi commented Apr 30, 2024

No description provided.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@y4ssi y4ssi changed the title Update CI with Github actions artifacts [WIP] Update CI with Github actions artifacts Apr 30, 2024
@y4ssi y4ssi changed the title [WIP] Update CI with Github actions artifacts Update CI with Github actions artifacts May 1, 2024
@y4ssi y4ssi requested a review from daira May 2, 2024 16:30
str4d
str4d previously approved these changes May 3, 2024
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

ACK a282f8a with questions and nits.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Comment on lines +200 to +201
${{ format('./src/test/test_bitcoin{0}', matrix.file_ext) }}
${{ format('./src/zcash-gtest{0}', matrix.file_ext) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the original GitHub Actions artifacts approach (that was replaced by GCA in be26acd), these particular binaries were stored in individual artifacts (with the rationale that we only needed one specific binary in the corresponding test jobs, so this would speed up the individual runners). Similarly, the other binaries were grouped based on what combinations were needed for testing. Does it work out to be simpler or faster to store all the binaries in one artifact?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, exactly, in the previous approach, we were optimizing. @daira had suggested combining them, which simplifies the code quite a bit. I'm satisfied with the time we've achieved, and I like the simplification. What I propose is to keep it for the sake of code simplicity/maintainability, and if in the future we see that the prices/time increase significantly or we're not satisfied, we can revisit it.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@str4d str4d added the A-CI Area: Continuous Integration label May 3, 2024
@str4d str4d added this to the Release 5.10.0 milestone May 3, 2024
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

ACK 1ab77a7

@nuttycom nuttycom merged commit f2823bc into master May 3, 2024
97 of 100 checks passed
@nuttycom nuttycom deleted the ga_artifacts branch May 3, 2024 19:21
@str4d str4d modified the milestones: Release 5.10.0, Release 5.9.1 May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants