Simplify if condition in build script

[rugk]

The of condition likely was wrongly inverted (it said it would run if the branch was not master). Also it's kinda useless anyway, as the trigger definitions above (in on) already restrict the branches.

[elrido]

These conditions reflect the one inside the buildx.sh script: https://github.com/PrivateBin/docker-nginx-fpm-alpine/blob/54165bb82e4e85896b40fa6121610e901b85979c/buildx.sh#L34-L39

The pipeline gets triggered under the following conditions:

• when a merge request is raised, running on the branch that should get merged to master
• when the master branch gets pushed (incl. changes that would get force-pushed directly into protected master)
• when a tag gets pushed
• on a schedule every night

My understanding of the purpose of those conditions is as follows: The pipeline runs builds and tests all the image flavours and architectures in all of the above cases, but new images only should get pushed when the run is not triggered by a merge request and either is triggered by:

1. the nightly schedule
2. pushing a tag

So I think that the condition is required to prevent pushes to master triggering pushing the image. The tag will be a GITHUB_REF of refs/tags/<x.y.z-etc>. It may be possible to express this condition differently than a "not master", if regexes or globs are an option in the github pipeline syntax. Or we could instead add a comment that spells out the intent of the conditions?

[rugk]

[ "${EVENT}" != pull_request ] && { \ 
         [ "${GITHUB_REF}" != refs/heads/master ] || \ 
         [ "${EVENT}" = schedule ] 
     } 

simplified:

  [ not pull_request ] && { \ 
         [ not refs/heads/master ] || \ 
         [ is schedule ] 
     } 

  not pull_request && ( not refs/heads/master || is schedule ) 

And in YAML:

(github.ref != 'refs/heads/master' && github.event_name != 'pull_request') || github.event_name == 'schedule'

(not 'refs/heads/master' && not 'pull_request') || is 'schedule'

---

Aka:

  not pull_request && ( not refs/heads/master || is schedule ) # sh
(not 'refs/heads/master' && not 'pull_request') || is 'schedule' # yaml

The brackets make these complete different if conditions...

• the first says: must not be a PR and (must either be a scheduled action or not on a master branch)
  • aka an image push is required when not a PR, but scheduled (so far as obvious); but this "not master" ah is just there to prevent master merges being pushed I get it
• the second: must be either a schedule or (both not on a master branch and not a pull request)
  • aka an container login is done when it is scheduled (okay), or even if it is not scheduled, but not a PR (obvious) and not on master

I dunno this is confusing and maybe a too optimized conditions…
Should not this be the same condition?

Okay, let's go full into that logic thing arg…

!P and ( !M or S ) # sh
(!M and !P) or S # yaml

me getting lost in thinking it needs to be inverted

Or ah the condition of needs to be inverted? But is this correct uhm? So let's use this old friend [de morgen thing](https://en.wikipedia.org/wiki/De_Morgan%27s_laws), and I hope I remember it correctly. If I invert `(!M and !P) or S` aka: ``` !((!M and !P) or S) = !(!M and !P) and !S = (M or P) and !S ``` So did I do a mistake or this is not the inverse?

Okay oh wait come on, it should be the same, you need a login when you push the image and vice versa, so what is wrong here?

!P and ( !M or S ) # sh
(!M and !P) or S # yaml

Let's forget the math thing (I give up lol), let's think what we want: The point about master is to avoid pushes or merges to the main branch to trigger a release? Fair enough, but that could be defined as this: github.event_name != 'pull_request' && (github.event_name == 'schedule')

Now, oh yeah, we want tags to trigger, I found that here and using the best practice there:

if: ${{ github.event_name != 'pull_request' && (github.event_name == 'schedule' || startsWith(github.ref, 'refs/tags/')) }}

I guess the confusion was about that negation and stuff. Expressing it in a "positive" way, i.e. when it should trigger is much easier to understand, so? I would propose this then? The same could be adjusted for the shell script...

[elrido]

Honestly, we have logic that works and when changing it there is a risk we break it (again - last time I tried changing it, I broke the nightly builds), so I wouldn't bother. But...

The cool thing about being a programmer is that we don't have to be good at doing the math ourselves, that's what the computers do for us. You could write a test_buildx.sh script that sources the buildx.sh script and runs a number of tests against the problematic function. Each test would set the relevant env vars and would check that the logic picks the right thing. Then you can safely play with the logic and rest assured that you didn't introduce any issues.

Since this is not the first time we had issues with that logic bit, it might be worthwhile adding such a test and tie it into .github/workflows/shellcheck.yml. It is a pity that we can't do something similar for the YAML condition.

[rugk]

Actually there are testing frameworks for shell scripts, so one could indeed write a unit test: https://shellspec.info/ (I would put it into a different testing YAML as shellcheck has a different purpose and is a static analyzer but well)

Anway it's off-topic, because as you said, we are talking about the YAML, which is not easy to test. The thing is if the shellscript does all the handling, do we need the YAML condition at all?

And if we have trouble with that piece again and again it just confirms that the current condition is not understandable and should be reworked.