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

Update instructions for local development #10

Merged
merged 3 commits into from
Nov 8, 2024
Merged

Conversation

paulineribeyre
Copy link
Collaborator

Improvements

  • Better local development support, and update instructions for local development

Copy link

github-actions bot commented Nov 7, 2024

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

@paulineribeyre paulineribeyre requested a review from nss10 November 7, 2024 20:25
Copy link
Contributor

@nss10 nss10 left a comment

Choose a reason for hiding this comment

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

Looks great! Just one small point—I think this line here also needs updating, as there’s no src/ in our project tree. Could you add that adjustment to this PR as well?

The rest of the changes look excellent and are definitely helpful for local testing. Thanks!

@@ -33,6 +33,10 @@ def get_app(httpx_client=None) -> FastAPI:
get_logger("gen3workflow", log_level=log_level)

logger.info("Initializing Arborist client")
if config["MOCK_AUTH"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming, Gen3Config would automatically ignore if there is no key MOCK_AUTH in the actual config, instead of throwing an error here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The default is False. Defaults are managed by gen3config.

@@ -42,25 +42,44 @@ To use a configuration file in a custom location, you can set the `GEN3WORKFLOW_

## Run Gen3Workflow

Update your configuration file to set `LOCAL_MIGRATION` to `true`, so that no attempts to interact with Arborist are made.
Copy link
Contributor

Choose a reason for hiding this comment

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

When was this implemented, I tried to even go back in the commit history to see this. Couldn't find it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a copy paste from requestor

Copy link
Contributor

Choose a reason for hiding this comment

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

😅 Gotcha!

@@ -64,8 +71,10 @@ async def authorize(
resources: list,
throw: bool = True,
) -> bool:
token = self.get_access_token()
if config["MOCK_AUTH"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is perfect! I was attempting to set up something similar locally to test some functionality. With this change, I won’t need to anymore—appreciate it!

@@ -8,6 +8,9 @@ DOCS_URL_PREFIX: /gen3workflow
# override the default Arborist URL; ignored if already set as an environment variable
ARBORIST_URL:

# /!\ only use for development! Allows running gen3workflow locally without Arborist interaction
Copy link
Contributor

Choose a reason for hiding this comment

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

Appreciate the warning.

@paulineribeyre paulineribeyre requested a review from nss10 November 7, 2024 20:56
Copy link
Contributor

@nss10 nss10 left a comment

Choose a reason for hiding this comment

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

Looks great. Approved

@paulineribeyre paulineribeyre merged commit dcd7a5c into master Nov 8, 2024
5 checks passed
@paulineribeyre paulineribeyre deleted the feat/local-dev branch November 8, 2024 16:27
nss10 pushed a commit that referenced this pull request Nov 11, 2024
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