.github, docs/jobs: update GitHub workflow and pre-build job#199
.github, docs/jobs: update GitHub workflow and pre-build job#199
Conversation
.github/workflows/build_rest_api.yml
Outdated
| needs: check-api | ||
| if: | | ||
| github.event_name == 'workflow_dispatch' || | ||
| needs.check-api.outputs.api_changed == 'true' |
There was a problem hiding this comment.
Correct me if I'm misunderstanding here. I believe the intent here is to rebuild documentation whenever something on snapd master has changed in the docs. I don't believe this workflow achieves that goal. In this case, it seems to me that tj-actions/changed-files will compare the latest commit on master against the one before. Since this workflow is running in a different repository (and will not run on every push to snapd master), then it will not catch all changes. It seems to me that the correct place for this is in snapd, where it knows when changes happen and can catch all of them.
It looks to me like, if you want to move building the docs artifact from the snapd repository, we would need to rebuild it always here and not check changed files.
What is the issue with the workflow running in the snapd repository? Is the pre-build-job.sh script not finding an artifact? I see that it is limiting the search to 100. We have a lot of pushes on master so perhaps that is not enough. If a larger search limit wouldn't help, then we could also potentially push the artifact to google and then this repo could grab the artifact from there.
So to summarize, I believe we have these options:
- Keep the docs build in snapd and only rebuild on doc changes, but either fix the
pre-build-job.shscript so it can always get the artifact or upload the artifact to a google bucket so it's always available. - Remove the docs build in snapd and rebuild the docs every single time a change happens in this repo.
Please let me know if I've misunderstood though.
There was a problem hiding this comment.
Yes this is incorrect, the intention here is to avoid the dependency to a snapd repo's workflow and thus, to always generate the openapi.json using the latest snapd's master in a PR job here in snapd-docs
Some background here.
To avoid the dependency between this job and ReadTheDocs build job, if possible it would be good to just execute these steps directly in the pre-build-job.sh to generate the artifact instead of trying to find it. The requirement would be that docker can be used in ReadTheDocs build job.
Looking at https://docs.readthedocs.com/platform/latest/build-customization.html, i think we could set up and use docker with pre_system_dependencies or post_system_dependencies (if the tools doesn't already support it)
Note: the docs build in snapd are currently not executed for master pushes but only for PR & manual workflow_dispatch. It will still remain so as there the intention is to build the docs with the PR changes.
There was a problem hiding this comment.
RTD build environment is already set up inside a docker container, and running docker inside of a docker container requires the outer container to be privileged, which I would assume is not the case on a production system.
Maybe the tools can be installed locally in the build environment.
There was a problem hiding this comment.
If we install locally, we have to use npm or npx, and because the containers are ephemeral, any install would have to occur on every run. This is not reliable, whereas with the workflow GitHub should cache the container
There was a problem hiding this comment.
From everything I can find RTD does not intend for you to do that kind of building on their server just the RTD workflow they support. Im pretty sure this is why I had to install GH manually in .readthedocs.yaml
There was a problem hiding this comment.
I need a fine-grained PAT for both the snap-docs and snapd repository with write access to workflows and contents for both repos. I don't have permission to create this. @sergiocazzolato would you be able to generate this?
There was a problem hiding this comment.
I think I see where you're going with this, and it sounds good to me. Do you already have a PR snapd side for triggering the new workflow here? Or what are you thinking there?
There was a problem hiding this comment.
Also, why do you need a PAT for this? the github token in the workflow can be given write access when run not from a fork. For example, in snapd this workflow https://github.com/canonical/snapd/blob/master/.github/workflows/rerun.yaml#L17 has permission to trigger others. No need for a PAT
There was a problem hiding this comment.
Aaaah I'm being silly, aren't I. Probably that token isn't valid for another repo. Never mind then.
There was a problem hiding this comment.
Ive been messing around with it. When I looked online it seemed like in order to use the dispatch with a custom event from another repo you needed a PAT with write access for both. I will try to look into this further
.github/workflows/build_rest_api.yml
Outdated
| needs: check-api | ||
| if: | | ||
| github.event_name == 'workflow_dispatch' || | ||
| needs.check-api.outputs.api_changed == 'true' |
There was a problem hiding this comment.
Yes this is incorrect, the intention here is to avoid the dependency to a snapd repo's workflow and thus, to always generate the openapi.json using the latest snapd's master in a PR job here in snapd-docs
Some background here.
To avoid the dependency between this job and ReadTheDocs build job, if possible it would be good to just execute these steps directly in the pre-build-job.sh to generate the artifact instead of trying to find it. The requirement would be that docker can be used in ReadTheDocs build job.
Looking at https://docs.readthedocs.com/platform/latest/build-customization.html, i think we could set up and use docker with pre_system_dependencies or post_system_dependencies (if the tools doesn't already support it)
Note: the docs build in snapd are currently not executed for master pushes but only for PR & manual workflow_dispatch. It will still remain so as there the intention is to build the docs with the PR changes.
| -R "${REPO}" \ | ||
| --workflow "${WORKFLOW}" \ | ||
| --status success \ | ||
| --branch master \ |
There was a problem hiding this comment.
Based on https://github.com/canonical/snap-docs/pull/199/changes#r2958816628 if we can directly generate the artifact here, we could remove this search from here.
But if keep it, note that this should not be master but it should be this exact branch otherwise there is no use of the above PR job
| WORKFLOW="build-documentation.yaml" | ||
| ARTIFACT_NAME="openapi-spec" # Updated to match the new GitHub Action output | ||
| REPO="canonical/snap-docs" | ||
| WORKFLOW="build-rest-api.yml" |
There was a problem hiding this comment.
It looks like you named your workflow with underscores instead of hyphens
| WORKFLOW="build-rest-api.yml" | |
| WORKFLOW="build_rest_api.yml" |
From SNAPDENG-36515