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

Store OpenAI API Key in config.json #17

Closed
wants to merge 1 commit into from

Conversation

honeymaro
Copy link
Contributor

@honeymaro honeymaro commented Feb 12, 2024

Summary

The main purpose of this PR is to modify the behavior of the CLI so that it generates a config.json file containing the API Key instead of fetching it from an environment variable (OPENAI_API_KEY). There are a couple of issues with using the environment variable approach:

Incompatibility with Other Programs: If other programs also rely on the OPENAI_API_KEY, they won’t be able to use different API keys independently.
Security Risk: Since OPENAI_API_KEY is a well-known key used by the OpenAI library, leaving it exposed in environment variables could lead to potential security risks. Malware or unauthorized access might exploit this key, resulting in unexpected charges.

Changes

  • Add LOZ prefix to OPENAI_API_KEY but keep OPENAI_API_KEY for a legacy.
  • Use readlinePromises instead of readline in src/config/index.ts and src/index.ts.
  • Remove the checkEnv method from src/index.ts as it is no longer needed.
  • Pass the apiKey to the OpenAiAPI constructor in src/llm/index.ts.
  • Remove the checkEnv test case from test/a.loz.test.ts as it is no longer needed.

Could you please review my pull request?
And thank you for making this project! Your hard work and dedication are greatly appreciated.

감사합니다. 새해 복 많이 받으세요.

- Add `LOZ` prefix to `OPENAI_API_KEY` but keep `OPENAI_API_KEY` for
  a legacy.
- Use `readlinePromises` instead of `readline` in `src/config/index.ts` and `src/index.ts`.
- Remove the `checkEnv` method from `src/index.ts` as it is no longer needed.
- Pass the `apiKey` to the `OpenAiAPI` constructor in `src/llm/index.ts`.
- Remove the `checkEnv` test case from `test/a.loz.test.ts` as it is no longer needed.
@joone
Copy link
Owner

joone commented Feb 12, 2024

Unable to run your changes due to a tsc error:
https://github.com/joone/loz/actions/runs/7871087508/job/21494074096?pr=17

Could you update your PR to avoid using readline-promise?

@honeymaro
Copy link
Contributor Author

Unable to run your changes due to a tsc error: joone/loz/actions/runs/7871087508/job/21494074096?pr=17

Could you update your PR to avoid using readline-promise?

I'm not sure if it can fix this problem or not,
Could we upgrade node.js and @type/node to v20 since ollama-node has dependency on v20? 🤔

@joone
Copy link
Owner

joone commented Feb 13, 2024

Merged. Thanks!

@joone joone closed this Feb 13, 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.

None yet

2 participants