Action var tests off

[BrianCashProf]

Reference Issues or PRs

What does this implement/fix?

Put a x in the boxes that apply

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds a feature)
• Breaking change (fix or feature that would cause existing features not to work as expected)
• Documentation Update
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build related changes
• Other (please describe):

Testing

• Did you test the pull request locally?
• Did you add new tests?

Any other comments?

Following the changes introduced in #2569, We have decided to use action variables instead of local environment variables.
These action variables must be set at the repo level and can be set by forks differently to the main repository without merge conflicts.
The action variable is set as a binary boolean (with 0 being false and 1 being true).

[viniciusdc]

Hi @BrianCashProf, thanks for opening the PR; I wonder if the variable could be boolean instead of the int. Also, while reading the docs for variables again, I noticed this:

If a configuration variable has not been set, the return value of a context referencing the variable will be an empty string.

So, one thing that we could also change is the primary condition, so instead of looking for a value of the given variable (and this requiring that to be preset), we could use:

if: ${{ ! vars.NO_PROVIDER_CREDENTIALS_*** }}

I've attested that this works as expected, but with the caveat that those variables must be set in the repository settings of a given fork.

[BrianCashProf]

Hi @BrianCashProf, thanks for opening the PR; I wonder if the variable could be boolean instead of the int. Also, while reading the docs for variables again, I noticed this:

If a configuration variable has not been set, the return value of a context referencing the variable will be an empty string.

So, one thing that we could also change is the primary condition, so instead of looking for a value of the given variable (and this requiring that to be preset), we could use:

if: ${{ ! vars.NO_PROVIDER_CREDENTIALS_*** }}

I've attested that this works as expected, but with the caveat that those variables must be set in the repository settings of a given fork.

I'll take a look into this method.

[BrianCashProf]

Hi @BrianCashProf, thanks for opening the PR; I wonder if the variable could be boolean instead of the int. Also, while reading the docs for variables again, I noticed this:

If a configuration variable has not been set, the return value of a context referencing the variable will be an empty string.

So, one thing that we could also change is the primary condition, so instead of looking for a value of the given variable (and this requiring that to be preset), we could use:

if: ${{ ! vars.NO_PROVIDER_CREDENTIALS_*** }}

I've attested that this works as expected, but with the caveat that those variables must be set in the repository settings of a given fork.

In my initial test I have realized I had used double quotes. Regular boolean works fine like such:


if: ${{ vars.NO_PROVIDER_CREDENTIALS_***  == ’false’ }}

The code you provided:

if: ${{ ! vars.NO_PROVIDER_CREDENTIALS_*** }}

Works well in the sense that if you do not set the variable at all it will run the pipeline but if you do set the variable to any value at all it will skip the pipeline.

If by default we want to run the pipeline and the flag only needs to be placed to skip the pipeline; then this is my suggestion:


if: ${{  vars.NO_PROVIDER_CREDENTIALS_*** != true }}
```


This way the following happen:
- If var not set, we run the test
- If var set to false, we run the test
- If var set to true, we skip the test

[tylergraff]

If by default we want to run the pipeline and the flag only needs to be placed to skip the pipeline; then this is my suggestion

@BrianCashProf Yes we do want to run by default and only skip if the flag is set accordingly. Please make your suggested change. Once that is committed, is there anything else needed before this PR can be merged?

[viniciusdc]

@BrianCashProf, make sure they follow the standard you suggested above. NO_PROVIDER_CREDENTIALS_AWS != 'true' to vars.NO_PROVIDER_CREDENTIALS_AZURE == 0. Also, now that we removed the previous check-for-credentials job and we are also relying on different variables for each provider anyway, I suggest that we rename the variable to SKIP_***_INTEGRATION_TEST.

[viniciusdc]

Is this still required?

[BrianCashProf]

Nope, I've removed the comment.

[viniciusdc]

LGTM, thanks @BrianCashProf