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

Enh 2451 skip conditionals #2569

Merged
merged 28 commits into from
Aug 2, 2024

Conversation

BrianCashProf
Copy link
Contributor

@BrianCashProf BrianCashProf commented Jul 19, 2024

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?

This PR is related to the issue stated in ENH: #2451
I have added a job to the integration test related github actions. This job acts as a mediary for a boolean environment variable and a conditional statement added to integration test job at it's top level. This was made so that a user with a fork of nebari who do not have access to the credentials for the cloud resources the job needs.
This method was chosen for the following reasons:

  • In testing, booleans on the step level did not prevent a failing of attempting to access the credentials.
  • In testing, environment variables could not be used to do a boolean on the job at large.
  • A flag that was not branch specific was requested by a few users (not stated in ticket) and this solution could provide this.
  • This avoids adding "push:" statements to the YAML (as opposed to using ignore-branches) which will keep the workflow inherently manual or on a cron job as dictated thus far.

Added a boolean environment variable which enables or disables running the aws integration tests.
Adds boolean environment variable so that user can toggle off the Azure integration test.
adds boolean environment variable so that user can toggle off the Digital Ocean integration test.
Adds boolean environment variable so that user can toggle off the GCP integration test.
removing white space
removed trailing white spaces
Trim trailing whitespaces
trim trailing whitespaces
trim trailing whitespaces
Trim Trailing Whitespace
@kcpevey kcpevey requested review from dcmcand and viniciusdc July 19, 2024 16:04
@kcpevey kcpevey added the needs: review 👀 This PR is complete and ready for reviewing label Jul 22, 2024
@viniciusdc
Copy link
Contributor

Hi, @BrianCashProf. All looks good, but I am curious about the extra steps of check-for-credentials_***: I noticed you use it to calculate the output used for the extra conditional in the steps later in the pipeline.

I don't see a problem with that, but if possible, we could instead use the env you set up in the beginning to control it directly, e.g.:

  test-aws-integration:
    runs-on: ubuntu-latest
    if: env.NO_PROVIDER_CREDENTIALS_aws == "false"
    permissions:
      id-token: write

@BrianCashProf
Copy link
Contributor Author

Hi, @BrianCashProf. All looks good, but I am curious about the extra steps of check-for-credentials_***: I noticed you use it to calculate the output used for the extra conditional in the steps later in the pipeline.

I don't see a problem with that, but if possible, we could instead use the env you set up in the beginning to control it directly, e.g.:

  test-aws-integration:
    runs-on: ubuntu-latest
    if: env.NO_PROVIDER_CREDENTIALS_aws == "false"
    permissions:
      id-token: write

That was my initial idea, but the Environment variables were not accessible when doing a boolean at the first level of the job.
if: env.NO_PROVIDER_CREDENTIALS_aws == "false" will produce an error that states it cannot read the env variables.
Next I tried adding the check on the token step itself, but it wouldn't stop the job from reaching out to a resource it did not have and erroring out.
With those not being possible, I thought of what variables could be read at that top level. Given that a job output could be read in the boolean, I used the environment variable to create a job output that could then be read in the boolean statement.

@viniciusdc
Copy link
Contributor

viniciusdc commented Jul 26, 2024

Indeed, it looks like the available contexts for the jobs.<id>.if conditional are only github, needs, vars, inputs. Here's a more detailed explanation.

Copy link
Contributor

@viniciusdc viniciusdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I don't see any problems with the extra job requirement, as its default allows backward compatibility already. Also, I like the fact that this will grant forks better stability.

@viniciusdc
Copy link
Contributor

viniciusdc commented Jul 29, 2024

@BrianCashProf, sorry for being pedantic here, but could you add a small comment in the new job explaining why it's in there? This way, we can easily track that in future maintenance of these files.

It could be something like:

# Used to skip cloud provider checks due to "jobs" not supporting {{ env }} variables contexts 

@BrianCashProf
Copy link
Contributor Author

BrianCashProf commented Jul 30, 2024

@viniciusdc No worries! That phrasing looks good to me. I've added the comments.
EDIT: Noticed I added a whitespace by accident. I submitted again with whitespace trimmed.

@viniciusdc viniciusdc modified the milestones: Next Release, 2024.7.1 Jul 31, 2024
@viniciusdc viniciusdc merged commit da50206 into nebari-dev:develop Aug 2, 2024
4 checks passed
@viniciusdc viniciusdc removed the needs: review 👀 This PR is complete and ready for reviewing label Aug 2, 2024
@BrianCashProf BrianCashProf deleted the ENH-2451-skip-conditionals branch August 9, 2024 17:42
@BrianCashProf BrianCashProf mentioned this pull request Aug 15, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

3 participants