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: ftltest config: use ConfigPaths with envar fallback #1543

Merged
merged 3 commits into from
May 21, 2024

Conversation

safeer
Copy link
Contributor

@safeer safeer commented May 20, 2024

Fixes #1502.

Adds tests to ensure we're correctly falling back to the config in FTL_CONFIG and to ftl-project.toml in the gitroot.

@safeer safeer requested a review from matt2e May 20, 2024 21:25
@safeer safeer requested a review from alecthomas as a code owner May 20, 2024 21:25
@safeer safeer requested review from a team and deniseli and removed request for a team May 20, 2024 21:25
@ftl-robot ftl-robot mentioned this pull request May 20, 2024
@safeer safeer force-pushed the ftltest-use-configpaths branch from c3cb181 to e31b468 Compare May 21, 2024 16:21
if ok {
paths = strings.Split(envValue, ",")
} else {
paths = pc.ConfigPaths(paths)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we confirm that paths isn't empty and return the same preprocessingErr error from before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention here is to have the same behavior as main.go, which allows LoadConfig to return an empty config after attempting to fall back on the envar, then the gitroot. I think having that parity makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. I was wondering because I can't think of a reason why someone would use WithProjectFiles without any paths. From that perspective, I'd find it useful to get the error message during development so that I know the way I'm using WithProjectFiles isn't actually doing anything. main.go wouldn't want to error in that case because the project toml isn't required, whereas if someone is explicitly invoking WithProjectFiles, I'd assume they have some intent to actually use a project toml.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's clear that if you're not passing explicit config files to WithProjectFiles() that whatever its described behaviour is is in effect. The ftl-project.toml file is designed to be the "default" for a project, and having it consistent across the CLI and tests is fine.

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.

nice!!

deniseli and others added 2 commits May 21, 2024 14:18
Follow up to #1539

We use `react-codemirror2`, which requires `codemirror` major version
`5.*`, even on their [master
branch](https://github.com/scniro/react-codemirror2/blob/master/package.json).
Since we have no reason to stop using `react-codemirror2`, we just need
to stop codemirror from continuing to get updated automatically by
renovate.

After this PR merges (assuming reviewers think this looks right!), I'll
close #1539.
@safeer safeer merged commit 0ed16e6 into main May 21, 2024
24 checks passed
@safeer safeer deleted the ftltest-use-configpaths branch May 21, 2024 21:29
@matt2e matt2e added the approved Marks an already closed PR as approved label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Marks an already closed PR as approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow module tests to be run from VS Code
4 participants