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(fal): support basic fal profiles #383

Merged
merged 1 commit into from
Jan 21, 2025
Merged

feat(fal): support basic fal profiles #383

merged 1 commit into from
Jan 21, 2025

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Jan 12, 2025

Currently one needs to either login or keep using FAL_KEY env vars either implicitly or explicitly, but both can lead to confusion. In my case I find myself generating new keys all the time, because I can't remember which one was which. This PR introduces

  • ~/.fal/config.toml or FAL_CONFIG_PATH with profile sections
  • FAL_PROFILE to set particular profile

which allows keeping keys in one place and then easily switching between them, similar to tools like aws cli or modal.

In the future we can also use this for acquiring list of teams that your account has access to and switching between them, similar to modal profile.

@squat
Copy link
Contributor

squat commented Jan 12, 2025

Instead of ~/.fal/config.toml can we default the config path to the more modern and manageable and less sprawling ~/.config/fal.toml( or ~/.config/fal/... in case there are more config files)? Also, rather than defaulting this to a hardcoded path, it would be best IMO to use whatever default is the convention for the given platform. E.g. on Linux the convention is to put app-specific config in ~/.config, anything else is weird, but this might be different for MacOS. So we could use https://pypi.org/project/platformdirs/ to automate this.

@efiop
Copy link
Contributor Author

efiop commented Jan 13, 2025

@squat I'm familiar with platformdirs/appdirs and used them before heavily, but I would say that they are more confusing for cli applications. They are great for GUI and other stuff as they obide XDG conventions, but it gets way more confusing for simple cli-oriented applications and I would rather not use them.

@squat
Copy link
Contributor

squat commented Jan 13, 2025

I ultimately don't care about platformdirs itself. My point is about using a config location that conforms to standards and user expectations, i.e. ~/.config/... (at least on Linux)

@efiop
Copy link
Contributor Author

efiop commented Jan 13, 2025

@squat The expectation is to have ~/.fal really, similar to ~/.aws. I totally understand your point about complying with the current best practices and not polluting the home dir and that IS what we all should do when working on system-programming kinds of tools, but my experience with user-facing cli tools tells me that it is better to keep it simple and in the home dir directly across all platforms. I was a big advocate of doing it properly and using appdirs/platformdirs and I did that for many years, and my conclusion is to KISS and keep it directly in home dir when working on user-facing cli tools.

@efiop efiop force-pushed the ruslan/fal-key branch 2 times, most recently from 22ee517 to 3b5c647 Compare January 13, 2025 16:18
Copy link
Member

@chamini2 chamini2 left a comment

Choose a reason for hiding this comment

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

Cool!

BTW, I agree we should just keep it in ~/.fal dir. if you are expected to modify the config, it should be very easy to find. Also, we already create the directory for Auth0 token

projects/fal/src/fal/auth/__init__.py Show resolved Hide resolved
@efiop efiop merged commit b7ec5a4 into main Jan 21, 2025
10 checks passed
@efiop efiop deleted the ruslan/fal-key branch January 21, 2025 20:31
@@ -77,6 +77,7 @@
"uvicorn",
"starlette_exporter",
"structlog",
"tomli",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, accident

efiop added a commit that referenced this pull request Jan 21, 2025
efiop added a commit that referenced this pull request Jan 21, 2025
efiop added a commit that referenced this pull request Jan 21, 2025
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