Skip to content

Conversation

@validbeck
Copy link
Collaborator

@validbeck validbeck commented Dec 2, 2024

Internal Notes for Reviewers

I THINK I finally refined it to the point where it will work. Some of the issue was the previous set-up defaults to always checking against main because that is the behaviour of how ${{ github.head_ref }} works:

  • Only available in the context of a PR and not a workflow push (So for example, when I tried to hack the deploy staging & docs workflows further because those are triggered by the merge-x-into-... workflows it ignored the input)
  • Always checks against the branch the PR was created from (in this case, main) even if you specify a different base

validate-docs-site.yaml

Now this checks the current PR against the branch you're intending to merge into (so it will in theory also work when we preview the site for pushes to prod and not just working PRs pre-main merge 🤞🏻):

# See if site/notebooks/ has updates
# Checks the current PR branch against the target branch
- name: Filter changed files
  uses: dorny/paths-filter@v2
  id: filter
  with:
    base: ${{ github.event.pull_request.base_ref }}
    ref: ${{ github.head_ref }}
    filters: |
      notebooks:
        - 'site/notebooks/**'

deploy-docs-staging.yaml & deploy-docs-prod.yaml

Other than the on.push.branches filter at the top of each workflow, these are now the same:

# See if site/notebooks/ has updates
# Checks against the previous commit to the branch prior to the current push event
- name: Filter changed files
  uses: dorny/paths-filter@v2
  id: filter
  with:
    base: ${{ github.event.before }}
    ref: ${{ github.sha }}
    filters: |
      notebooks:
        - 'site/notebooks/**'

We are now checking for changes against the PREVIOUS commit prior to the push that triggered the workflow. HOPEFULLY??? this is the final solution??? Allegedly ${{ github.sha }} is available to push workflows so that should solve our issue.

Expected behaviour

PR > main

    • site/notebooks/** has a change pushed up to the PR, it will trigger the notebook execution: SUCCESSFUL WORKFLOW RUN

main > staging

  • After this PR is merged into main, the workflow merging into staging from main will trigger the notebook execution:

staging > prod

  • The next time we publish to prod, the workflow merging into staging from prod will trigger the notebook execution:

@validbeck validbeck added the internal Not to be externalized in the release notes label Dec 2, 2024
@validbeck validbeck self-assigned this Dec 2, 2024
@validbeck validbeck changed the title I'm strong Fixing on the fly notebook execution filter Dec 2, 2024
@validbeck validbeck requested a review from nrichers December 2, 2024 20:29
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2024

PR Summary

This pull request updates the GitHub Actions workflows for deploying documentation to production and staging, as well as validating the documentation site. The main change involves modifying the base reference used in the dorny/paths-filter action to compare against the previous commit rather than a specific branch name. This change ensures that the workflows accurately detect changes in the site/notebooks/ directory by comparing the current commit against the commit prior to the current push event. Additionally, a minor change was made to the site/notebooks/README.md file to remove a test comment.

Test Suggestions

  • Verify that the GitHub Actions workflows correctly identify changes in the site/notebooks/ directory when new commits are pushed.
  • Test the deployment process to both production and staging environments to ensure no disruptions occur due to the updated base reference.
  • Check that the validation workflow for the documentation site functions as expected with the new base reference.
  • Ensure that the removal of the test comment in site/notebooks/README.md does not affect any documentation generation or display.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2024

A PR preview is available: Preview URL

Copy link
Collaborator

@nrichers nrichers left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@validbeck validbeck merged commit 542db1c into main Dec 3, 2024
5 checks passed
@validbeck validbeck deleted the beck/not-so-quick-workflow-fix branch December 3, 2024 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Not to be externalized in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants