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

Pelican decouple build workflow #3404

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

noelmiller
Copy link

@noelmiller noelmiller commented Oct 8, 2024

Pull Request Checklist

Resolves: #3403

  • Ensured tests pass and (if applicable) updated functional test output
  • Conformed to code style guidelines by running appropriate linting tools
  • Added tests for changed code
  • Updated documentation for changed code

@seanh seanh self-assigned this Oct 11, 2024
seanh added a commit to seanh/ghp-pelican-demo that referenced this pull request Oct 14, 2024
seanh added a commit to seanh/ghp-pelican-demo that referenced this pull request Oct 14, 2024
seanh added a commit to seanh/ghp-pelican-demo that referenced this pull request Oct 14, 2024
Combine `build.yml` and `github_pages.yml` into a single workflow file.
Having `build.yml` be a separate workflow file that `github_pages.yml`
calls has two problems:

1. All the inputs (and their types, descriptions, defaults) have to be
   duplicated in the two workflow files.

2. The reference to `@main` in `uses: "getpelican/pelican/.github/workflows/build.yml@main"`
   in the `github_pages.yml` workflow makes development and testing
   awkward.

For example let's say you want to send a pull request that contains
changes to `build.yml` and that also requires changes to
`github_pages.yml` at the same time. The problem is that the
`github_pages.yml` on your PR branch will still contain the reference to
`build.yml` `@main` so if you try to test the branch's version of
`github_pages.yml` it'll call the version of `build.yml` from `main` not
from the PR branch and will fail.

This commit move the build job from `build.yml` into `github_pages.yml`
but makes the deploy job optional: it adds a `deploy` input and will
only run the deploy job if `deploy = true` is passed.

Previously a user could test their build without deploying it by calling
the separate `build.yml` workflow. Now they can do it by calling the
`github_pages.yml` workflow and passing `deploy=false`.

This commit also moves the `concurrency` stuff from the workflow level
to the deploy level: it's okay to have multiple build jobs running
concurrently, just not multiple deploy jobs.
Copy link
Contributor

@seanh seanh left a comment

Choose a reason for hiding this comment

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

Hi @noelmiller,

I've given this a test and it works but I think there are two problems with my idea to split out a separate build.yml file, so I'm very sorry to have wasted your time by getting you to do that:

  1. As you've pointed out you have to duplicate all of the inputs and their descriptions, types, etc in both files.
  2. The reference to @main in uses: "getpelican/pelican/.github/workflows/build.yml@main" in the github_pages.yml workflow makes development and testing awkward. For example let's say you want to send a pull request that contains changes to build.yml and that also requires changes to github_pages.yml at the same time. The problem is that the github_pages.yml on your PR branch will still contain the reference to build.yml @main so if you try to test the branch's version of github_pages.yml it'll call the version of build.yml from main not from the PR branch and will fail. This could be worked around but it's awkward.

So I think we should go back to having both jobs in the one github_pages.yml file as you had it originally, sorry! I've sent you a PR to do this: noelmiller#1. Take a look at my PR and if you agree, please merge it into this PR and re-request a review from me.

Aside from that, the only other issue I see is adding docs explaining how to test your build without deploying it, but I think that can be done in a separate PR later.

Future thoughts

In the future if we ever want to add support for more deployment platforms (not just GitHub Pages) we can add those as additional optional jobs in the same workflow file.

The naming of the workflow as github_pages.yml is a bit unfortunate now because the workflow doesn't always deploy to GitHub Pages, if you're only calling it to test the build or if (in future) you're calling it to deploy to a different platform then your workflow run doesn't actually have anything to do with GitHub Pages. A different name would probably be better but renaming the file would break any existing calling workflows that're using the current name. I think we'd have to put a github_pages.yml workflow that just prints out an error message telling people to update their caller workflows (or perhaps it could delegate to the new workflow name, passing through all the arguments). I don't think we should rename the workflow now, let's just leave it as-is for now, just a thought for the future.

…le-file

Combine `build.yml` and `github_pages.yml`
@noelmiller
Copy link
Author

noelmiller commented Oct 17, 2024

@seanh

I like this approach a lot better. Where would the best place be to update the documentation regarding this? Or do you want to do that as a separate PR?

Oh, I missed this part: "Aside from that, the only other issue I see is adding docs explaining how to test your build without deploying it, but I think that can be done in a separate PR later."

I am still happy to update the docs to explain this, just having trouble tracking down where I can update that in your documentation.

@noelmiller noelmiller requested a review from seanh October 17, 2024 17:05
@seanh
Copy link
Contributor

seanh commented Oct 17, 2024

Where would the best place be to update the documentation regarding this? Or do you want to do that as a separate PR?

I'm actually a new contributor to Pelican myself, the GitHub Actions workflow is the only thing I've contributed.

I think you could either add the documentation changes to this PR or do them as a separate PR, either way seems fine to me.

The https://docs.getpelican.com/en/latest/tips.html page in the docs comes from the https://github.com/getpelican/pelican/blob/main/docs/tips.rst file in this repo, so that's the file you want to change. There's already docs in that file about how to use the workflow to deploy to GitHub Pages, so you just need to add a section explaining how to use the workflow to test the build only. Using the workflow just to test the build doesn't have anything in particular to do with GitHub Pages so I wouldn't put the new documentation in the GitHub Pages section. I'd maybe add a new top-level section to the file about using GitHub Actions to test site generation.

@seanh
Copy link
Contributor

seanh commented Oct 17, 2024

Oh, I forgot to say if you would like to preview your changes to the docs locally you can run:

tox -e docs

...to build the docs and then run:

python3 -m http.server -d docs/_build/html/

...to serve the docs, and open http://localhost:8000/

@justinmayer
Copy link
Member

If you have invoke installed, there is a built-in task for building, watching, and serving:

invoke docserve

@noelmiller
Copy link
Author

noelmiller commented Oct 18, 2024

Added a new commit to update the docs. Any feedback?

If you have invoke installed, there is a built-in task for building, watching, and serving:

invoke docserve

Side Tangent:
So I went down quite a rabbit hole trying to figure out how to set up my python environment (I am more of a sysadmin than a dev). I'm not sure if this Containerfile would be useful to folks? I work off a Fedora Atomic based desktop Bazzite, so installing dependencies on my local machine is not usually a thing I like to do.

FROM cgr.dev/chainguard/python:latest-dev

WORKDIR /app

ADD ./requirements /app/
COPY tasks.py /app/

VOLUME /app

USER root

RUN apk update \
    && apk upgrade

RUN python -m venv /root/virtualenvs/pelican

ENV PATH="/root/virtualenvs/pelican/bin:${PATH}"

RUN pip install -Ur developer.pip \
    && pip install -Ur noel.pip # Includes invoke we could obviously include invoke in the developer.pip

RUN invoke tools

ENTRYPOINT ["invoke"]

This is what I ended up writing. Not sure how useful it would be? Works great for docs.

To build: podman build . -t pelican-source
To run: podman run -p 8000:8000 -v ./:/app:Z pelican-source:latest docserve

Equivalent docker commands would be:
docker build . -f ./Containerfile -t pelican-source
docker run -p 8000:8000 -v ./:/app pelican-source:latest docserve

@noelmiller noelmiller marked this pull request as ready for review October 18, 2024 21:30
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

Thank you for the documentation enhancement. Seems good to me, aside from a few minor suggested changes 👍

docs/tips.rst Outdated Show resolved Hide resolved
docs/tips.rst Outdated Show resolved Hide resolved
docs/tips.rst Outdated Show resolved Hide resolved
docs/tips.rst Outdated Show resolved Hide resolved
@justinmayer
Copy link
Member

@seanh: Any thoughts on the current state of this pull request?

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.

Decouple Deployment to Github Pages from Published Action
3 participants