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

fix(docs): docs published with incorrect version number + api docs missing after release #1066

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Aug 19, 2022

Description of your changes

As described in #1063 there were two issues related to the docs publishing after a release. Both issues were due to oversights on my part.

Issue 1 - API docs are missing

The first one was that I removed a step from the reusable-publish-docs (link) and this step was the one that published the API docs under the correct version number in the docs. You can verify this by checking the /latest version of the docs, which works (while the v1.1.0 doesn't). Adding back the step fixes the docs missing issue.

Issue 2 - Incorrect version number

The second one was instead due to a behavior of GitHub Actions that I misunderstood.

For context: we have 2 workflows: make-release (link) and reusable-publish-docs (link).

The first one is triggered manually, the second is called by the former as the last step (we also use it elsewhere, hence it’s reusable- prefix).

During the make-release workflow we perform some actions, one of these is to increment the version of our packages, commit, & push a tag.

When make-release calls reusable-publish-docs I was expecting the latter to checkout the code that contains the latest changes & tag, so that it can build the docs using the correct version number.

What happens instead is that when checking out the actions/checkout action detects the latest tag (L517 of “Checkout code step” here) but then the code in the repo & the tag used are the previous & same as the one in the make-release context.

Below a diagram that visually represents what described so far:
IMG_0009

Reading the docs on reusing workflow though, there are some info that I was missing. Below two key excerpts:

A workflow that uses another workflow is referred to as a "caller" workflow. The reusable workflow is a "called" workflow. One caller workflow can use multiple called workflows.

and

For example, if the called workflow uses actions/checkout, the action checks out the contents of the repository that hosts the caller workflow, not the called workflow.

This means that even though the called workflow (reusable-publish-docs) was correctly detecting the presence of a new tag, the context used and the the version of the repo checked out, were the same as the make-release one, which was in a point in time before the release & consequent version bump.

There are two solutions that come to mind here:

  1. Have the caller workflow export the commit SHA of the post-version state of the repo, pass it to the called workflow, and have the called workflow to use said SHA to checkout the correct version of the workflow. This approach would be laborious and would make the two workflow tightly coupled, reducing the reusability of the called workflow.
  2. Change the trigger of the called workflow and execute it in response to an actual release event. This is an event that that is triggered after a maintainer publishes a release, which means that the docs will be synced to the GitHub release instead of the the NPM publish. This is the approach that I am proposing in this PR which also removes both issues described above since it'll get always the latest tag in the event.

Finally, I'm also adding a way to manually trigger the publish docs workflow as a break-glass solution that will allow us to manually trigger a docs publish if any of the other triggers fails.

How to verify this change

Review the code & see these workflow runs in a fork:

  • I run make-release - result here - (only difference here is that I'm not publishing to NPM, otherwise the workflow is the same)
  • I then make a release on the GH repo (manually) - (This is a release in the release section, not a workflow execution)
  • This triggers the reusable-publish-docs workflow - result here
  • Check that the docs correspond to the released version - link (I'll delete this after the PR is merged to avoid confusion)

Additionally, I have also run the publish docs workflow manually, here's the result.

Related issues, RFCs

Issue number: #1063

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • The PR title follows the conventional commit semantics

Breaking change checklist

N/A


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dreamorosi dreamorosi self-assigned this Aug 19, 2022
@dreamorosi dreamorosi linked an issue Aug 19, 2022 that may be closed by this pull request
@github-actions github-actions bot added the bug Something isn't working label Aug 19, 2022
@dreamorosi dreamorosi marked this pull request as ready for review August 19, 2022 13:57
Copy link
Contributor

@ijemmy ijemmy left a comment

Choose a reason for hiding this comment

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

@dreamorosi Thanks for the detailed description about the issues!

Let me double check my understanding.

Reusable Publish docs workflow can be triggered from 3 scenarios:

  1. (GitHub) Release (publish-docs-on-releases.yml)
  2. Manual trigger (from publish-docs-on-releases.yml)
  3. Merge to main (line 26 in on-merge-to-main.yml)

Is that correct?

If that is the case, can the issue 2 (checking out the wrong version) also happens in merge to main scenario?

@@ -18,6 +18,8 @@ jobs:
secrets:
token: ${{ secrets.GITHUB_TOKEN }}
run-unit-tests:
needs: get_pr_details
if: ${{ needs.get_pr_details.outputs.prIsMerged == 'true' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: why do we need prIsMerged to be true? I thought this workflow is triggerred only when PR merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This workflow is actually triggered by record_pr.yml, which in turn is triggered every time a PR is opened, updated (title or description change), or closed. This is a design that we copied from the Powertools Python repo.

The reason why we don't want to trigger directly is that this workflow is somewhat privileged and we don't want to allow forks to access it. So we trigger the workflow that exports the PR details for most events, and then dispatch the other downstream workflows. Then those workflows can decide what to do based on the content/status of the PR.

@dreamorosi
Copy link
Contributor Author

dreamorosi commented Aug 22, 2022

Is that correct?

Yes, your understanding is correct.

If that is the case, can the issue 2 (checking out the wrong version) also happens in merge to main scenario?

No, because when we merge on main we don't make any commit / create any tags, so the context of the repository in the called workflow is the same as the one that triggered it (which is the merge action). Also in this one we don't care about the version since we publish the docs always to dev. The docs are published under a specific version only when we make a release.

@dreamorosi dreamorosi requested a review from ijemmy August 22, 2022 11:17
@dreamorosi dreamorosi merged commit 8b8b25c into main Aug 23, 2022
@dreamorosi dreamorosi deleted the 1063-bug-docs-incorrect-version-number-in-the-docs-+-api-docs-returning-404 branch August 23, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: incorrect version number in the docs + API docs returning 404
3 participants