Default to use poetry on push and PR

[teutoburg]

This is necessary because input parameters only exist for workflow_call and workflow_dispatch, so it's not really possible to set a default value when the action is triggered by a push or pull request. However, since the sample project also uses poetry, those triggers should use it as well. This should solve that.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c4fdeac) to head (b1ed68e).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #51   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines            7         7           
=========================================
  Hits             7         7           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[teutoburg]

Alright, this was a lot of work, although simple in concept. Understanding how exactly GitHub actions interprets various things is hard, and so is the syntax in these workflow files. But I think I now tested all possible cases and it seems to do what it should. That is, use the input flag when called from elsewhere or manually and default to using poetry when triggered internally from a PR or push. The admittedly hacky trigger input parameter seemed to be the only way (found in an extensive discussion of a similar issue) to distinguish between the cases "workflow_call from an PR" and "on pull_request".

[teutoburg]

Once this is reviewed, I'm going to squash merge this. There's no reason to keep all the debug commits in here, but I found it impossible to test this locally...

[hugobuddel]

OK, let's try to break this down, because I don't understand at all what we're trying to do and how we are accomplishing it. I do understand the total clusterduck github actions is. So I fully trust you, and am trying to learn.

Just thinking out loud here, so I can check whether I understand this.

Default to use poetry on push and PR

Sounds good! Now I'm wondering why we don't use poetry all the time though. I understand some projects don't use poetry, but those never use it, and the rest can always use it. At least that is what I naively would think.

do what it should. That is, use the input flag when called from elsewhere or manually and default to using poetry when triggered internally from a PR or push.

I'm not sure, but I think this is referring to the USE_POETRY flag.

This is necessary because input parameters only exist for workflow_call and workflow_dispatch, so it's not really possible to set a default value when the action is triggered by a push or pull request. However, since the sample project also uses poetry, those triggers should use it as well.

Ah, OK, so the USE_POETRY flag is not always available. So we can't always use it and we need a more reliable mechanism to determine whether poetry should be used.

The admittedly hacky trigger input parameter seemed to be the only way (found in an extensive discussion of a similar issue) to distinguish between the cases "workflow_call from an PR" and "on pull_request".

I fully appreciate this frustration. You are way more patient than I am. I got too frustrated. E.g. AstarVienna/METIS_Pipeline#104 is technically incorrect because it uses github parameters that are not always set, but it kinda works.

Now onto trigger. I fully expect that you figured out that this works; I'm now just trying to understand it.

      trigger:
        type: string
        description: "Fake input parameter to tell if triggered from outside. Do not set manually."
        required: false
        default: "external"

I don't know what 'triggered from outside' or 'external' means. I guess that workflow_dispatch counts as 'from the outside', because then the user starts it, but not sure about the rest. Oh, I can just see what is what, by looking at where it is Ah, workflow_call, which is from another, 'external', repository. So it means something different from what I thought.

Now

      - name: Decide if poetry
        id: decide
        env:
          USE_POETRY: "${{ (((github.event_name == 'pull_request') || (github.event_name == 'push')) && (inputs.trigger != 'external')) || ((inputs.poetry == true) && ((github.event_name == 'workflow_dispatch') || (inputs.trigger == 'external'))) }}"
        run: echo "USE_POETRY=$USE_POETRY" >> "$GITHUB_OUTPUT"

Aaaaaah, too many braces! Let's break this down:

(
    (
        (github.event_name == 'pull_request')
       ||
        (github.event_name == 'push')
    ) 
    &&
   (inputs.trigger != 'external')
)
||
(
    (inputs.poetry == true)
    && 
    (
        (github.event_name == 'workflow_dispatch')
        ||
        (inputs.trigger == 'external')
    )
)

So poetry is used in two cases:

• trigger is not external (or not set I suppose), and we're dealing with either a
  • pull request or
  • push
• USE_POETRY is set, and either
  • this is a workflow_dispatch, or
  • trigger is external

Let's see whether I can rephrase that to when poetry is not used, to verify I understand correctly.

Poetry is not used when both these conditions hold:

• Condition 1:
  • trigger is external (so a workflow_call), or
  • this is neither pull request or push (so workflow_dispatch is also allowed)
• AND Condition 2:
  • USE_POETRY is false (or unset), or
  • not a dispatch and not trigger is external, so not a dispatch or call

I'm still not sure that I fully understand this though... But I must stop here. Maybe you can use my confusion above to improve documenting what is going on a bit more?

Not sure whether I would be able to properly maintain this if you would win the lottery, but I'll take that chance.

[hugobuddel]

Once this is reviewed, I'm going to squash merge this. There's no reason to keep all the debug commits in here, but I found it impossible to test this locally...

I feel your pain. To commemorate it and let your grandchildren witness this horror from before their time, I'm going to share this screenshot:

(And now let's hope that my grandchildren look back with embarrassment that I'm still using gmail.)

[teutoburg]

do what it should. That is, use the input flag when called from elsewhere or manually and default to using poetry when triggered internally from a PR or push.

I'm not sure, but I think this is referring to the USE_POETRY flag.

Almost. It's actually referring to the poetry input parameter (which becomes inputs.poetry once we're inside the actual workflow). But that one is a horrible mess because there's apparently no way to give it a default value when the workflow is not triggered by workflow_call or workflow_dispatch. Oh and don't get me started on the fact that these input parameters need to be defined twice when you want to use both. Why not just define input parameters in a separate section at the top and then they get a default value when triggered via e.g. push or pull_request or schedule? But anyway, "it is what it is".

So in the case that the workflow is triggered by a "non-parameterable event", inputs.poetry is the empty string (again, why not the default value??). But the empty string is not only "truthy", it's also (unlike in e.g. python) actually == true and also == 'true'. Yeah, not quite as bad as javascript but close. So I introduced the manually set USE_POETRY flag over which I have a lot more control, although for some reason the bool values I put there always end up as strings. But never the empty string, so I can at least consistently condition from it afterwards.

Aaaaaah, too many braces! Let's break this down:

(
    (
        (github.event_name == 'pull_request')
       ||
        (github.event_name == 'push')
    ) 
    &&
   (inputs.trigger != 'external')
)
||
(
    (inputs.poetry == true)
    && 
    (
        (github.event_name == 'workflow_dispatch')
        ||
        (inputs.trigger == 'external')
    )
)

Right. I wasn't sure how well the workflow file would handle line breaks here, so I just avoided them. I think I tried a line-by-line version already at the beginning somewhere (feel free to look through the commits...), but that didn't work, potentially for other reasons. Error messages in here are often not the most helpful.

On this trigger parameter: I found out (through tr(y|ial) and error(s) and reading several discussions somewhere) that the context of a workflow is inherited. Which actually makes a lot of sense, and we're relying on that for many other things I think. But in practice, this means that whenever the workflow is called via workflow_call, say from another repo (which is the primary use of these DevOps workflows anyway), the github.event_name is still whatever triggered it there, so e.g. pull_request or push in many cases. But then there's no way to distinguish the case that the workflow was triggered by a PR in this repo itself (other than that the inputs.poetry parameter is the empty string, but I found no reliable way to check for that, as it seems that true == '' as well??). So I found this solution in another discussion elsewhere, which actually included several people stating that this was the only way they could make their workflows, well, work, in similar situations. Oh and I originally tried to use a bool flag called external for this, but ran into the same issue that I couldn't reliably check whether it was unset. But with a string, this works fine, because '' != 'external' fortunately.

I suppose another way to approach this could have been to "normalize" the workflows such that the actual workflow is only triggered by workflow_call (and perhaps workflow_dispatch, which also has parameters, that even work the same now (it seems bool were treated as string in one but not the other in the past)) and we have another "rump" workflow that's triggered by push and PR, that in turn calls that, similar to how these DevOps workflows are usually triggered "in the wild" from other repos. On that note, the webtests workflow is now not tested here on push or PR, mostly I guess because we don't simulate webtests in here. I might actually end up going down that route eventually.

Sounds good! Now I'm wondering why we don't use poetry all the time though. I understand some projects don't use poetry, but those never use it, and the rest can always use it. At least that is what I naively would think.

It occurred to me halfway through all of this, that we don't have many repos left that use these workflows (IRDB doesn't) that don't use poetry. After all, it most likely would have been quicker to just convert those to poetry and ditch the non-poetry part and by extension the whole input parameter. But hey, if we ever run into a case like this again, we now know how to solve it! I should write it down somewhere...

[teutoburg]

I feel your pain. To commemorate it and let your grandchildren witness this horror from before their time, I'm going to share this screenshot:

I disabled those emails long ago 😆

[hugobuddel]

I didn't mean to imply that we should split up the conditional over multiple lines in the code, I just did that in the comment to figure out how it worked. I still don't fully understand, but I trust you

[teutoburg]

I didn't mean to imply that we should split up the conditional over multiple lines in the code, I just did that in the comment to figure out how it worked. I still don't fully understand, but I trust you

Yeah no, I would be nice to do that, but in the end I don't think we'll look at this piece of code too often. There were just too many other things going on during the conference that kept me from merging this...