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: validate project config #1955

Merged
merged 6 commits into from
Jul 4, 2024
Merged

fix: validate project config #1955

merged 6 commits into from
Jul 4, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Jul 3, 2024

Validates project config files.

This breaks previous cases where ftl config set and similar would be able to create the project toml if it wasn't already created. Now that a project's name is required, an invalid project config would have been created.

It is now a requirement to go through ftl init <name> <dir>

@matt2e matt2e requested a review from alecthomas as a code owner July 3, 2024 06:17
@matt2e matt2e requested review from a team and wesbillman and removed request for a team July 3, 2024 06:17
@ftl-robot ftl-robot mentioned this pull request Jul 3, 2024
@matt2e matt2e force-pushed the matt2e/validate-project-config branch 2 times, most recently from 213a13d to fbe94b9 Compare July 3, 2024 06:50
Copy link
Collaborator

@wesbillman wesbillman left a comment

Choose a reason for hiding this comment

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

Do we need to update the root ftl-project.toml for FTL as well?

I'm torn by the need to give the project a name when using ftl init but I guess it's inline with other tools like npm init.

@@ -61,7 +61,7 @@ func tempConfigPath(t *testing.T, existingPath string, prefix string) string {
var existing []byte
var err error
if existingPath == "" {
existing = []byte{}
existing = []byte("name = \"generated\"")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: You might be able to use backticks here:

`name = "generated"`

@@ -47,6 +47,26 @@ func (c Config) Root() string {
return filepath.Dir(c.Path)
}

// Validate checks that the configuration is valid.
func (c Config) Validate() error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a test for this one, or is it covered by other tests that use Create?

@matt2e
Copy link
Collaborator Author

matt2e commented Jul 3, 2024

Do we need to update the root ftl-project.toml for FTL as well?

I'm torn by the need to give the project a name when using ftl init but I guess it's inline with other tools like npm init.

This was done in a previous PR: #1952 👍

@matt2e matt2e force-pushed the matt2e/validate-project-config branch from b5db7a2 to 0b15159 Compare July 3, 2024 23:54
@matt2e matt2e enabled auto-merge (squash) July 3, 2024 23:54
@matt2e matt2e merged commit c5ddd13 into main Jul 4, 2024
45 of 47 checks passed
@matt2e matt2e deleted the matt2e/validate-project-config branch July 4, 2024 00:00
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.

2 participants