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

fix: GetModuleContext: use config from context instead of from envar #1534

Merged
merged 1 commit into from
May 20, 2024

Conversation

safeer
Copy link
Contributor

@safeer safeer commented May 17, 2024

Fixes #1352

With this change, the behavior is:

  • ftl dev examples/go
    • uses ftl-project.toml in the gitroot if it exists
  • FTL_CONFIG=ftl-project-inline-a.toml ftl dev examples/go
    • uses ftl-project-inline-a.toml
  • ftl dev examples/go --config=ftl-project-inline-b.toml
    • uses ftl-project-inline-b.toml
  • FTL_CONFIG=ftl-project-inline-a.toml ftl dev examples/go --recreate --config=ftl-project-inline-b.toml
    • uses ftl-project-inline-b.toml

@safeer safeer requested review from alecthomas and a team as code owners May 17, 2024 22:56
@safeer safeer requested review from deniseli and removed request for a team May 17, 2024 22:56
@ftl-robot ftl-robot mentioned this pull request May 17, 2024
@safeer safeer requested a review from wesbillman May 17, 2024 23:09
@safeer safeer marked this pull request as draft May 18, 2024 00:02
@safeer safeer marked this pull request as ready for review May 20, 2024 16:53
…stead of creating a new managers using the envar file
@safeer safeer force-pushed the controller-getmodulecontext-use-ctx branch from 3de44bd to 339dddf Compare May 20, 2024 16:55
@deniseli
Copy link
Contributor

For your fourth case:

  • FTL_CONFIG=ftl-project-inline-a.toml ftl dev examples/go --recreate --config=ftl-project-inline-b.toml
    • uses ftl-project-inline-b.toml

Have we discussed whether that should instead merge the contents of ftl-project-inline-a.toml and ftl-project-inline-b.toml? The configuration manager is set up to operate on multiple configs, so I wasn't sure intuitively whether we want the --config flag to take strict precedence over the envar one, or if the behavior should be additive.

Great job knocking this out in any case!

@safeer
Copy link
Contributor Author

safeer commented May 20, 2024

Thanks @deniseli! I had a conversation with @wesbillman and @worstell, and we all agreed that having the CLI path override the envar makes the most sense, since it's more explicit. I'll hold off until we can talk about it during the sync :)

Copy link
Contributor

@deniseli deniseli left a comment

Choose a reason for hiding this comment

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

Sweet, thanks @safeer! That makes complete sense. I'll go ahead and approve so you aren't blocked on merging. If y'all already talked through it, then it sounds like this is ready to go :)

The only remaining thought I had was whether it'd be useful to log some sort of warning if we happen to override the envar with the flag, in case someone's using the flag without realizing there's also an envar config. That's low enough impact, though, that even if we did want to do that, you are more than welcome to do that in a followup :)

@safeer
Copy link
Contributor Author

safeer commented May 20, 2024

Looks like the override actually happens separately in Kong :)

@safeer safeer merged commit 5a62849 into main May 20, 2024
24 checks passed
@safeer safeer deleted the controller-getmodulecontext-use-ctx branch May 20, 2024 21:41
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.

ftl-project.toml loaded even if a different project file is specified
2 participants