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 config validation not erroring on global options in wrong scope #10950

Merged
merged 2 commits into from Oct 13, 2020

Conversation

Eric-Arellano
Copy link
Contributor

This used to not complain:

[source]
backend_packages = ["pants.backend.python"]

Closes #3713.

[ci skip-rust]
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

I plan to cherry-pick to 1.30.2.

This means that an option belonging to [cache] will now error if it's in [cache.java]. @benjyw is that correct behavior?

@coveralls
Copy link

coveralls commented Oct 12, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling 104ebdd on Eric-Arellano:config-validation into 247fa16 on pantsbuild:master.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Oct 12, 2020

I plan to cherry-pick to 1.30.2.

This means that an option belonging to [cache] will now error if it's in [cache.java]. @benjyw is that correct behavior?

No, that is incorrect behavior. [cache.java] accepts all options that [cache] does - it is simply the instance of cache scoped to java. So do not cherry-pick...

@Eric-Arellano
Copy link
Contributor Author

Hm, then this PR is incorrect. Until we remove the "enclosing scope" logic, we should probably special case the validation logic to ignore GLOBAL as a valid enclosing scope in this particular context. I'll revisit.

Copy link
Sponsor Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

I think this is a fine short-term fix. Long-term, options and subsystems probably need some reworking and simplification, even if we want scoped subsystems to continue to exist.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano merged commit 348b228 into pantsbuild:master Oct 13, 2020
@Eric-Arellano Eric-Arellano deleted the config-validation branch October 13, 2020 18:04
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Oct 13, 2020
…antsbuild#10950)

This used to not complain:

```toml
[source]
backend_packages = ["pants.backend.python"]
```

Closes pantsbuild#3713.

[ci skip-rust]
[ci skip-build-wheels]
Eric-Arellano added a commit that referenced this pull request Oct 14, 2020
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.

Verify config doesn't explode for valid option names in the wrong sections
4 participants