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

chore: Update skip condition for integration tests #260

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

jstlaurent
Copy link
Contributor

Changelogs

  • Update the skip check to skip if either required settings are missing

Checklist:

  • Was this PR discussed in an issue? It is recommended to first discuss a new feature into a GitHub issue before opening a PR.
  • Add tests to cover the fixed bug(s) or the newly introduced feature(s) (if appropriate).
  • Update the API documentation if a new function is added, or an existing one is deleted.
  • Write concise and explanatory changelogs above.
  • If possible, assign one of the following labels to the PR: feature, fix, chore, documentation or test (or ask a maintainer to do it for you).

The previous check would pass if at least one of username or password was set, and the test would fail.

@jstlaurent jstlaurent requested a review from cwognum as a code owner January 31, 2025 16:17
@jstlaurent jstlaurent self-assigned this Jan 31, 2025
Copy link
Collaborator

@cwognum cwognum left a comment

Choose a reason for hiding this comment

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

Thanks @jstlaurent ! I'm surprised that this has been working locally but now fails in the CICD. Is this needed because os.getenv() returns an empty string rather than None for PRs opened by external contributors? To predict our CICD secrets, presumably?

@jstlaurent jstlaurent force-pushed the chore/tweak-integration-test branch from 4a325a7 to 09dca52 Compare January 31, 2025 17:54
@jstlaurent
Copy link
Contributor Author

Thanks @jstlaurent ! I'm surprised that this has been working locally but now fails in the CICD. Is this needed because os.getenv() returns an empty string rather than None for PRs opened by external contributors? To predict our CICD secrets, presumably?

Yep. When a PR comes from a fork, the secrets are present as environment variables, but they contain an empty string. Why GitHub chose this, I honestly don't understand... When using os.getenv(), the variable exists, so the None default value does not get returned. And since we test for is None, the test passes.

I updated the Pydantic settings class to env_ignore_empty=True, so empty values will use the default of None set in the class for these environment variables.

If we need to expose secrets to external contributors, we can create deployment environments and invite folks there. That way, we can control who gets access. I don't think we need this right now, but it's an option for the future.

@jstlaurent jstlaurent merged commit 6747c58 into main Jan 31, 2025
19 checks passed
@jstlaurent jstlaurent deleted the chore/tweak-integration-test branch January 31, 2025 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants