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

Build + push preview image only on master branch #6507

Merged
merged 1 commit into from
Oct 8, 2023

Conversation

guidopetri
Copy link
Contributor

What type of PR is this?

  • Bug Fix
  • Other

Description

The CI is trying to build + push the preview tagged image on PRs, but we only want that to happen on the master branch. This change adds a condition to the CI step that runs the build+push, filtering for the ref to be master as a condition of the CI step being run.

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Added a test commit that skips the e2e/unit tests (for faster iteration) and verified that the build+push CI step does not run on this PR.

Related Tickets & Documents

#6503 , #6505 , #6420

@guidopetri guidopetri force-pushed the gpetri/build-only-on-master branch from d576464 to 086f665 Compare October 7, 2023 17:34
@@ -172,7 +181,7 @@ jobs:
- frontend-unit-tests
- frontend-e2e-tests
- build-skip-check
if: ${{ needs.build-skip-check.outputs.skip }} == false
if: needs.build-skip-check.outputs.skip == 'false'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out we don't need the {{ }} jinja braces, and this is now actually filtering whereas it was not before.

@guidopetri guidopetri enabled auto-merge (squash) October 7, 2023 17:36
@guidopetri
Copy link
Contributor Author

@justinclift when you have a minute, mind giving this a look?

@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Merging #6507 (086f665) into master (0c2dc4e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6507   +/-   ##
=======================================
  Coverage   61.10%   61.10%           
=======================================
  Files         158      158           
  Lines       12883    12883           
  Branches     1753     1753           
=======================================
  Hits         7872     7872           
  Misses       4765     4765           
  Partials      246      246           

if [[ "${{ vars.DOCKER_USER }}" == '' ]]; then
echo 'Docker user is empty. Skipping build+push'
echo skip=true >> "$GITHUB_OUTPUT"
elif [[ "${{ secrets.DOCKER_PASS }}" == '' ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Good idea. The extra messages will help us figure out what's going wrong, when something is not working right. 😄

Copy link
Member

@justinclift justinclift left a comment

Choose a reason for hiding this comment

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

Looks good to me. 😄

@guidopetri guidopetri merged commit 0f175b7 into master Oct 8, 2023
16 checks passed
@justinclift justinclift deleted the gpetri/build-only-on-master branch October 8, 2023 00:55
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Jan 8, 2025
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.

2 participants