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

use matrix for CircleCI config #2864

Merged
merged 13 commits into from
Sep 16, 2021
Merged

use matrix for CircleCI config #2864

merged 13 commits into from
Sep 16, 2021

Conversation

cce
Copy link
Contributor

@cce cce commented Sep 9, 2021

Summary

Based on the matrix job @algojack added in #2749, this turns the other per-platform jobs into matrix jobs too. The most significant change in this PR is switching from building in the home directory (/home/circleci on Linux and /Users/distiller on Mac) to instead build out of /opt/cibuild to make it easier for the matrix jobs to share configuration.

Test Plan

It would be good to check that all the same tests are being run as before, perhaps by going through the build output manually or the artifacts (resulting JSON/XML files).

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2021

Codecov Report

Merging #2864 (2695b21) into master (0cfe715) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2864      +/-   ##
==========================================
+ Coverage   47.28%   47.31%   +0.02%     
==========================================
  Files         355      355              
  Lines       56901    56901              
==========================================
+ Hits        26908    26923      +15     
+ Misses      26958    26948      -10     
+ Partials     3035     3030       -5     
Impacted Files Coverage Δ
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
network/requestTracker.go 70.25% <0.00%> (-0.87%) ⬇️
network/wsNetwork.go 61.09% <0.00%> (+0.18%) ⬆️
network/wsPeer.go 74.65% <0.00%> (+0.27%) ⬆️
ledger/acctupdates.go 62.63% <0.00%> (+0.83%) ⬆️
catchup/peerSelector.go 100.00% <0.00%> (+1.04%) ⬆️
ledger/blockqueue.go 85.05% <0.00%> (+2.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cfe715...2695b21. Read the comment docs.

Copy link
Contributor

@algobarb algobarb left a comment

Choose a reason for hiding this comment

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

Read through it and reviewed the CircleCI results. It seems good to me and it introduced me to yaml syntax for using &ref_name and <<: *ref_name. The extended use of matrices for the executors does reduce a lot of duplicate code.

I wonder how easy it will be for someone new to jump into and make changes to the config file later on and whether the YAML is enough or if we need any documentation on what we expect our CircleCI to currently do.

@cce
Copy link
Contributor Author

cce commented Sep 13, 2021

Yes, I also didn't know that YAML syntax either until I saw it recently in some CircleCI docs and then read CircleCI's YAML guide on Anchors and aliases.

@algojack
Copy link
Contributor

lgtm, here's a good simple article on the << & and * https://medium.com/@kinghuang/docker-compose-anchors-aliases-extensions-a1e4105d70bd

@algojack
Copy link
Contributor

@cce is the GIMME change in the second file on purpose? what does it do exactly?

@cce
Copy link
Contributor Author

cce commented Sep 13, 2021

The original build.sh was downloading via curl the "gimme" script to ~/gimme.sh, so I changed it to check for a GIMME_INSTALL_DIR and use that, falling back to $HOME as before if it is not set. That made it possible for the matrix build not to be so dependent on the value of $HOME.

@algojohnlee algojohnlee merged commit bc46c94 into master Sep 16, 2021
@algojohnlee algojohnlee deleted the matrix-circleci-config branch September 16, 2021 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants