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

chore: Update docker-build-publish.yml #2838

Merged
merged 7 commits into from
Oct 12, 2023
Merged

chore: Update docker-build-publish.yml #2838

merged 7 commits into from
Oct 12, 2023

Conversation

MSevey
Copy link
Member

@MSevey MSevey commented Oct 11, 2023

Overview

This fixes the bug that rollkit local-celestia-devent was experiencing with the docker images being overwritten.

The changes to the reuseable docker action simplify the logic for easier maintenance.

The changes to the node CI file are to only run the docker action on a PR or pushes to main. When the workflow was triggering on all push events, the action was running twice on PRs which was contributing to the confusion and causing images to be overwritten.

This means if the team wants a docker image built for testing, they must open at least a draft PR. The PR doesn't need to be merged, but the PR trigger is what will built the image.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@github-actions github-actions bot added the external Issues created by non node team members label Oct 11, 2023
@MSevey MSevey marked this pull request as draft October 11, 2023 20:07
@MSevey
Copy link
Member Author

MSevey commented Oct 11, 2023

sorry, syntax bug slipped through. Not sure why it didn't come up in my testing.

Fix is here, celestiaorg/.github#78

Will do another release and update this PR.

@ramin ramin added the kind:ci CI related PRs label Oct 12, 2023
@MSevey MSevey marked this pull request as ready for review October 12, 2023 14:08
@MSevey MSevey marked this pull request as draft October 12, 2023 14:09
@MSevey
Copy link
Member Author

MSevey commented Oct 12, 2023

man this PR is giving me a run for my money haha

@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2023

Codecov Report

Merging #2838 (8c7f5b2) into main (7491d53) will decrease coverage by 0.33%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2838      +/-   ##
==========================================
- Coverage   51.20%   50.87%   -0.33%     
==========================================
  Files         168      168              
  Lines       10730    10730              
==========================================
- Hits         5494     5459      -35     
- Misses       4755     4790      +35     
  Partials      481      481              

see 8 files with indirect coverage changes

@MSevey MSevey marked this pull request as ready for review October 12, 2023 14:52
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

utack

@MSevey MSevey self-assigned this Oct 12, 2023
@Wondertan Wondertan merged commit f1a302b into main Oct 12, 2023
15 of 17 checks passed
@Wondertan Wondertan deleted the MSevey-patch-1 branch October 12, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Issues created by non node team members kind:ci CI related PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants