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

Add manual workflow to deploy docker images to blockstack #285

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AshtonStephens
Copy link
Collaborator

@AshtonStephens AshtonStephens commented Oct 13, 2023

Summary of Changes

Add manual docker image publishing

Testing

Risks

Few risks. It could provide faulty images and make people believe their code is broken when it's just the images.

How were these changes tested?

Tested on an x86 computer. Has issues with ARM64

What future testing should occur?

Should find a way to generate arm64 images.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@stjepangolemac
Copy link
Contributor

How would this play out locally if we pull some changes, try spinning up devenv and the images are not built yet in the CI?

@AshtonStephens
Copy link
Collaborator Author

It would play out poorly - you would need to run ./build.sh locally to overwrite the ones here. For most users, this will be fine.

It might be worth making a separate devenv repo that doesn't only pulls in the images. At a point, our code should be stable

@AshtonStephens AshtonStephens force-pushed the build-docker-images branch 2 times, most recently from 42beb8a to 799a66d Compare October 13, 2023 14:47
@AshtonStephens AshtonStephens marked this pull request as ready for review October 13, 2023 14:47
@AshtonStephens AshtonStephens self-assigned this Oct 13, 2023
@AshtonStephens AshtonStephens force-pushed the build-docker-images branch 2 times, most recently from 1be5589 to fe36b57 Compare October 13, 2023 15:18
.github/workflows/publish-to-dockerhub.yml Outdated Show resolved Hide resolved
password: ${{ secrets.DOCKERHUB_PASSWORD }}
- name: Build and push Docker image
uses: docker/build-push-action@v5
if: ${{ matrix.service }} == sbtc
Copy link
Contributor

Choose a reason for hiding this comment

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

Is sbtc not in the matrix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sBTC is above, because it's built with the sources here I put it separately.

Copy link

Choose a reason for hiding this comment

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

same question as @CAGS295 - is there a reason sbtc can't be added to this matrix that i'm not seeing here?
the build step looks compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

if: ${{ matrix.service }} != sbtc No?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the check means to only run if it is not sbtc. Otherwise, I don't understand the purpose of the if condition.

.github/workflows/publish-to-dockerhub.yml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (66e9238) 41.03% compared to head (a906eec) 42.70%.
Report is 25 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
+ Coverage   41.03%   42.70%   +1.66%     
==========================================
  Files          40       45       +5     
  Lines        5042     5290     +248     
  Branches        0       47      +47     
==========================================
+ Hits         2069     2259     +190     
- Misses       2973     3030      +57     
- Partials        0        1       +1     
Flag Coverage Δ
unittests 76.61% <ø> (?)

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.

platform: amd64

push_services_to_registry:
strategy:
Copy link

Choose a reason for hiding this comment

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

Suggested change
strategy:
strategy:
fail-fast: false
max-parallel: 4 # or 8,10 etc

Copy link

@wileyj wileyj left a comment

Choose a reason for hiding this comment

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

some minor changes that might be good to make, but the workflow isn't complicated so I'll approve it. suggested changes/questions are not blockers

@friedger
Copy link
Collaborator

Is this good to merge?

@friedger
Copy link
Collaborator

@AshtonStephens This is still helpful for devenv when using nakamoto. Can we have docker images for the current state of stacks-blockchain node?

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2023

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

None yet

6 participants