Merge queue creates spurious tag dirs in documentation repo

[infotroph]

Bug Description

The proximal culprit seems to be line 67 of book.yml, which assumes any ref with a slash in it must be a tag. But it's possible we need to instead/additionally tweak the conditions under which the deployment is triggered (possibly if: github.event_name == 'push' instead of if: github.event_name != 'pull_request'?)

[chethanagopinath]

Hi, this is my first time contributing to this repo. Could I pick this one up?

[mdietze]

@chethanagopinath Yes please, that'd be great

[chethanagopinath]

@mdietze just had some context questions to start:

1. Looking at the documentation repo, is it complete setup and context documentation for each version?
2. Also wondering when we would want to update that documentation repo?

Thanks.

[infotroph]

1. Yes, the documentation repo contains compiled versions of the manual for the entire PEcAn system (as opposed to the R function docs for individual packages, which live in the PEcAn repo). There is a version (each in its own folder) to correspond to each tagged release of PEcAn plus the up-to-the-minute develop version.
2. This repo is automatically updated (By GitHub Actions via book.yml) every time we merge changes to a corresponding branch/tag of the PEcAn repo. The problem is that GitHub's new and very handy "merge queue" feature creates a lot of temporary branches that don't need to be preserved as artifacts, so the task here is to find the cleanest way for this deployment script to ignore temporary references like the merge queue branches without missing tags that need preserving.

[harshagr70]

Hey !! i want to work on this issue .. can i proceed ??

[infotroph]

@harshagr70 Yes, please do! Let us know if you need more info.

[allgandalf]

@harshagr70 , if you need any help, be sure to ping me on slack, also read #3216 as both are related :)

[harshagr70]

@harshagr70 , if you need any help, be sure to ping me on slack, also read #3216 as both are related :)

Sure thing !! We can definitely discuss it !!

[Sweetdevil144]

I've been looking into the workflow setup for our documentation repo and noticed the merge queue is generating some extra directories we don't need. I'd like to tackle this issue and have a couple of suggestions to put forward. export VERSION=${GITHUB_REF##*/} as specified by @infotroph , This line treats any GITHUB_REF with a slash as a tag, but we know branches can contain slashes too, leading to our current problem. I'm thinking of tightening up the logic here to clearly identify tags. We could check if the GITHUB_REF actually starts with refs/tags/ Also, I believe we should revisit the conditions for the deployment step. Right now, it's set to run when it's not a pull request:

if: github.event_name != 'pull_request'

Maybe we could be more specific, such as:

if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/')

This way, we ensure that the deployment is only triggered for tag pushes, which is when we actually want to update our documentation.

I'd love to get your thoughts on this and work together to refine our process.

[infotroph]

@Sweetdevil144 Yes, I think this is the right approach.

We do also want to deploy on every push to develop or master -- will your proposal do that already or will we ned to add those explicitly? And am I right that adding it explicitly would be a matter of matching on any of refs/tags/, refs/heads/master, refs/heads/develop?

[Sweetdevil144]

Hey!

You're spot on. My proposal was primarily focused on the tagging issue, but we definitely need to ensure that pushes to develop and master trigger deployments as well. To achieve this, we can extend the conditional to include those branches explicitly. Here’s how we could adjust the workflow to handle it:

on:
  push:
    branches:
      - master
      - develop
    tags:
      - '*'

And for the deployment step, we could indeed match explicitly like this:

if: >
  github.event_name == 'push' && 
  (startsWith(github.ref, 'refs/tags/') || 
  github.ref == 'refs/heads/master' || 
  github.ref == 'refs/heads/develop')

Now, I'm not that good when dealing with yaml files but this should cover our bases for both tag creation and direct pushes to master and develop. Does this align with our deployment strategy, or are there additional scenarios we need to account for? I'll need to have a complete overlook upon the workflow for more information. If I'm missing something, do correct me please.

Thanks for the guidance!

[infotroph]

@Sweetdevil144 Thanks for the detailed proposal! I think what you wrote there is very close to being the whole patch needed -- would you like to open a PR that makes those changes in book.yml?

Does this align with our deployment strategy, or are there additional scenarios we need to account for?

I believe these are all of them, but we can get confirmation from @robkooper before merging the PR.

[Sweetdevil144]

@infotroph Sure !! I'll open a PR as soon as I redesign the book.yml file according to results which we want, of course only manipulating the line which need to be. Before that, can you confirm all deployment strategies so that we get a clear idea of what's needed to be worked upon?

[Sweetdevil144]

Hey @infotroph , did you confirm all the strategies? Rest of work is done and I'm ready to draft a PR !! ^^

[infotroph]

I've confirmed these are all the strategies we use at the moment, so go ahead with the PR--we can always add more later when we discover we need them.

[Sweetdevil144]

@infotroph Created a PR for the same!! Do remind me if any further changes need to be made!!

[PoppingPixel9]

Closed