lfit / github2gerrit

Github PR to Gerrit Change request
Other
2 stars 3 forks source link

Mixed/confused usage of workflow inputs/variables #16

Closed ModeSevenIndustrialSolutions closed 1 week ago

ModeSevenIndustrialSolutions commented 1 week ago

The primary workflow YAML code has a clear problem with the usage of the ORGANIZATION variable:

https://github.com/lfit/github2gerrit/blob/main/.github/workflows/github2gerrit.yaml

Here on line 267 we have:

              -F owner=${{ vars.ORGANIZATION }} \

Here on line 493 we have:

            const output = `The pull-request PR-${{ env.PR_NUMBER }} is submitted to Gerrit [${{ inputs.ORGANIZATION }}](https://${{ env.GERRIT_SERVER }})! \n

In previous test cases, this has probably worked because vars.ORGANIZATION (set at either the ORG or REPO level in the ODL GitHub account) and the ORGANIZATION variable passed to the workflow, have both been set and are the same. I will raise a PR to fix the inconsistency.

Also, we can improve the handling of ORGANIZATION by doing the following:

        required: false
        default: ${{ github.repository_owner }}

In the majority of cases, this should remove the need to explicitly set the ORG name as a variable (in most cases that is already set to the required value), whilst still allowing it to be set explicitly as an input. Minimising the number of workflow inputs and setting sane default values is a good practice for us to adopt.

ModeSevenIndustrialSolutions commented 1 week ago

PR raised here: https://github.com/lfit/github2gerrit/pull/17