GothenburgBitFactory / bugwarrior

Pull github, bitbucket, and trac issues into taskwarrior
http://pypi.python.org/pypi/bugwarrior
GNU General Public License v3.0
732 stars 209 forks source link

validation is somewhat counter productive #874

Closed srl295 closed 2 years ago

srl295 commented 2 years ago

Background

849 added validation of the config file, which is great, but, it also is a bit too picky. For example:

[github_srl295]  <- Unrecognized section 'github_srl295'. Did you forget to add it to 'targets' in the [general] section?

Problem

However, since there's not a way to specify certain sections from the command line, commenting out a section at a time is a normal way that I decide "I just want to update one section and not others", especially when debugging an issue with one section.

Suggestion

Note: the following sections weren't mentioned in 'targets' under [general]: github_srl295, …

--targets github_srl295 (could just override the 'targets' option)

[github_srl295]
skip=true
ryneeverett commented 2 years ago

Thanks for the report. I wanted to merge #849 relatively early in the release cycle in order to solicit feedback like this.

I'd be in favor of downgrading to a warning.

srl295 commented 2 years ago

dispite this comment https://github.com/ralphbean/bugwarrior/issues/923#issuecomment-1119163966 actually, using --flavor still hits this regression:

[github_srl295]  <- Unrecognized section 'github_srl295'. Did you forget to add it to 'targets' in the [flavor.myflavor] section?
ryneeverett commented 2 years ago

dispite this comment #923 (comment) actually, using --flavor still hits this regression:

[github_srl295]  <- Unrecognized section 'github_srl295'. Did you forget to add it to 'targets' in the [flavor.myflavor] section?

I'm pretty sure the "flavor" sections have to be named "flavor.X". I don't think you'd hit this error if the section was named "flavor.github_srl295". See bugwarrior.config.load.remove_inactive_flavors.

srl295 commented 2 years ago

@ryneeverett a more complete example:

[general]
targets = fee, fie, foe, fum

[flavor.just_fum]
targets = fum

[fee]
service = trello
trello.api_key = 000
trello.token = 000

[fie]
service = jira
jira.base_uri = http://0.0.0.0
jira.username = 000
jira.password = 000

[foe]
service = github
github.token = 000
github.username = srl295
github.login = srl295

[fum]
service = github
github.token = 000
github.username = srl295
github.login = srl295

with no options it fails with the expected login issues (as it should!)

with --flavor just_fum I still get complaints about the sections not listed in the flavored targets=fum

3 validation errors found in /Users/srl295/.config/bugwarrior/bug874
See https://bugwarrior-docs.readthedocs.io

[fee]  <- Unrecognized section 'fee'. Did you forget to add it to 'targets' in the [flavor.just_fum] section?

[fie]  <- Unrecognized section 'fie'. Did you forget to add it to 'targets' in the [flavor.just_fum] section?

[foe]  <- Unrecognized section 'foe'. Did you forget to add it to 'targets' in the [flavor.just_fum] section?
srl295 commented 2 years ago

BTW i did make a crack at fixing this myself (because code talks and we're all busy). So far I can either set extra = 'allow' in the pydantic settings or cause the validation to still fail. Any hints on downgrading to a warning?

ryneeverett commented 2 years ago

From a quick scan of schema.py I'm not sure the value of the warnings would be worth the extra complication and it might be best to simply ignore these cases. I think the simplest way would be to change the lines

class SchemaBase(pydantic.BaseSettings):
    Config = PydanticConfig

to

class SchemaBase(pydantic.BaseSettings):
    class Config(PydanticConfig):
        # Allow extra top-level sections so all targets don't have to be selected.
        extra = 'ignore'

I think we could also delete the remove_inactive_flavors method and invocation because I believe it's only purpose was to avoid these "extra" errors.

srl295 commented 2 years ago

From a quick scan of schema.py I'm not sure the value of the warnings would be worth the extra complication and it might be best to simply ignore these cases. I think the simplest way would be to change the lines

class SchemaBase(pydantic.BaseSettings):
    Config = PydanticConfig

to

class SchemaBase(pydantic.BaseSettings):
    class Config(PydanticConfig):
        # Allow extra top-level sections so all targets don't have to be selected.
        extra = 'ignore'

OK. will it still complain about, say, extra fields in Jira config? because that seems more useful.

I think we could also delete the remove_inactive_flavors method and invocation because I believe it's only purpose was to avoid these "extra" errors.

Flipping this on its head, another option would be an optional "skip these"

targets = fee, fie, foe, fum
skip = fee, fie, foe

or an optional "only run this"

targets = fee, fie, foe, fum
only = fum

that would leave the validation as is

ryneeverett commented 2 years ago

Yes, I believe all the fields within sections sections would still be strictly validated because PydanticConfig.extra would still be "forbid" and PydanticConfig is the base class of Config in both the service schemas and the main section schema. My suggestion should only override extra in the top level schema which validates section names.

srl295 commented 2 years ago

@ryneeverett great i will try that.

srl295 commented 2 years ago

@ryneeverett thanks, i hadn't gotten to it yet