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

feat: initial changes for support for isDevelopmentMode=enabled #192

Merged
merged 19 commits into from
Nov 28, 2024

Conversation

waveywaves
Copy link
Contributor

@waveywaves waveywaves commented Nov 25, 2024

  • Skip firebase init in dev mode.
  • Add logging for AuthService in dev mode
  • Disable PortKey initialization in dev mode

at the time of commit, both ENV and isDevelopmentMode are used interchangeably across the
codebase to check if the application is in development mode. This change ensures that
we are only checking one environment variable as a single source of truth to check if the
application is in development mode or not.
@vineetshar
Copy link
Contributor

Hi @waveywaves some thoughts towards this pull request.

The separation between ENV and isDevelopmentMode is intentional in its naming and use case.

The ENV signifies the environment the app is running in, signfying configuration to load in dev / stage or prod env.

The isDevelopmentMode is a bit different in required use case, it tells the app that it can run without dependencies such as firebase, GitHub configuration etc.

The ENV development still needs firebase / gcp / GitHub etc because it simply is about running potpie's backend locally. But isDevelopmentMode disables all these to bring in support for lrunning without most dependencies and support local parsing for example, and other things in future.

You might need to redo some stuff in the PR for this.

@waveywaves
Copy link
Contributor Author

Hey @vineetshar, ah okay. Thanks for helping me out with that. I'll update this info in the contributing doc in that case if it isn't there.

@waveywaves
Copy link
Contributor Author

@vineetshar I have updated the PR, lmkwyt

app/main.py Outdated Show resolved Hide resolved
app/main.py Outdated Show resolved Hide resolved
@waveywaves
Copy link
Contributor Author

@vineetshar the PR is ready for review. Thank you in advance.

app/modules/intelligence/provider/provider_service.py Outdated Show resolved Hide resolved
app/main.py Show resolved Hide resolved
app/modules/utils/posthog_helper.py Outdated Show resolved Hide resolved
@waveywaves
Copy link
Contributor Author

@dhirenmathur thank you for looking into this, updated based on your review

@waveywaves waveywaves changed the title feat: initial changes for support for ENV="development" feat: initial changes for support for isDevelopmentMode=enabled Nov 27, 2024
@vineetshar
Copy link
Contributor

@dhirenmathur thank you for looking into this, updated based on your review

@waveywaves one feedback left about user creation, can we do that as well please.

@waveywaves
Copy link
Contributor Author

@vineetshar yes, shall revert to the comment asap

@vineetshar
Copy link
Contributor

Approving this since disabling the mentioned things work as part of this PR. But development mode will be fully functional once we support parsing (which is to be done today 28th Nov), before that it is not really usable but is outside the scope of this specific pull request.

Copy link
Contributor

@vineetshar vineetshar left a comment

Choose a reason for hiding this comment

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

LGTM

@vineetshar vineetshar merged commit ccfd06c into potpie-ai:main Nov 28, 2024
1 of 2 checks passed
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.

3 participants