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

jupyter extension - add tests for env var configuration #9809

Merged
merged 2 commits into from Mar 8, 2024

Conversation

kevyang
Copy link
Contributor

@kevyang kevyang commented Mar 7, 2024

@kevyang kevyang requested a review from bbonenfant March 7, 2024 21:49
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Kevin Yang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@bbonenfant bbonenfant left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -664,11 +665,11 @@ def setup_handlers(web_app, config_file: Path):
f"Created Pachyderm client for {client.address} from local config"
)
except FileNotFoundError:
if PACHD_ADDRESS:
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this PR, but just clarifying. We want to use pachd_address/dex_token passed through env vars only if no config file is present? And then it doesn't appear that this is written to a config file - should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is correct. good call on this not writing to config file, i think it makes sense to write it. i have added that logic and added testing for it.

@@ -30,7 +30,18 @@ def pach_config(request, tmp_path) -> Path:


@pytest.fixture
def app(pach_config) -> tornado.web.Application:
def pach_config_env_var(request) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit of a nit, but this name seems misleading to me, and the marker name seems a little vague. Could they be something like pachd_address_env_var?

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.

None yet

3 participants