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

Mixed/confused usage of workflow inputs/variables #16

Closed
ModeSevenIndustrialSolutions opened this issue Oct 15, 2024 · 1 comment · Fixed by #17
Closed

Mixed/confused usage of workflow inputs/variables #16

ModeSevenIndustrialSolutions opened this issue Oct 15, 2024 · 1 comment · Fixed by #17

Comments

@ModeSevenIndustrialSolutions
Copy link
Contributor

ModeSevenIndustrialSolutions commented Oct 15, 2024

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 ModeSevenIndustrialSolutions changed the title Mixed/confused usage of workflow invputs/variables Mixed/confused usage of workflow inputs/variables Oct 15, 2024
@ModeSevenIndustrialSolutions
Copy link
Contributor Author

PR raised here: #17

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 a pull request may close this issue.

1 participant