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

sdk_test: add environment variable tests #6420

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

robhafner
Copy link
Contributor

Add two environment variable tests which illustrate how an environment variable can be used in OPA to verify a JWT.

Why the changes in this PR are needed?

What are the changes in this PR?

Notes to assist PR review:

Further comments:

srenatus
srenatus previously approved these changes Nov 17, 2023
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! Just one nitpick re: os.Setenv vs t.SetEnv

sdk/opa_test.go Outdated
@@ -2150,6 +2151,150 @@ result := {
}
}

func TestOpaRuntimeEnvironmentVariableDefinedInOS(t *testing.T) {
os.Setenv("TOKEN_VERIFY_KEY", "B41BD5F462719C6D6118E673A2389")
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Let's use t.SetEnv

@robhafner
Copy link
Contributor Author

I updated the commit on my forked branch expecting it to update the PR, but it doesn't look like it did. Is there something else that I need to do for the PR to be updated?

main...robhafner:opa:envvars

@robhafner
Copy link
Contributor Author

robhafner commented Nov 17, 2023

I think I was looking at an outdated compare. I think it looks correct now.

https://github.com/open-policy-agent/opa/compare/c8217ece91e6f540fd2d449d0f068c81b9e0d153..ffed5e7de5b9498fcc5412e4f2f67c3f88856ef9

srenatus
srenatus previously approved these changes Nov 17, 2023
sdk/opa_test.go Outdated
defer server.Stop()

testBundleResource := "/bundles/bundle.tar.gz"
testLabel := "a label"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the label relevant to the test? I'd suggest removing it if not... 🤔 Sorry I didn't notice in the last round of reviewing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not relevant. I removed it from both tests and updated the PR. Thanks!

Add two environment variable tests which illustrate how an environment variable can be used in OPA to verify a JWT.

Signed-off-by: Robert Hafner <[email protected]>
@srenatus srenatus merged commit 2ba3930 into open-policy-agent:main Nov 17, 2023
24 checks passed
colinjlacy pushed a commit to colinjlacy/opa that referenced this pull request Nov 20, 2023
Add two environment variable tests which illustrate how an environment variable can be used in OPA to verify a JWT.

Signed-off-by: Robert Hafner <[email protected]>
Signed-off-by: Colin Lacy <[email protected]>
colinjlacy pushed a commit to colinjlacy/opa that referenced this pull request Nov 22, 2023
Add two environment variable tests which illustrate how an environment variable can be used in OPA to verify a JWT.

Signed-off-by: Robert Hafner <[email protected]>
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 this pull request may close these issues.

2 participants