Nasir.thomas/AI-5146/ddev validate config fields

[ddog-nasirthomas]

What does this PR do?

• Non-existent option or value-level fields in spec.yaml are detected and reported when running ddev validate config.
• The default field behavior is clarified for users by ensuring that both default_display and default are properly respected.
• Fix existing spec.yaml fields with incorrect option or value level fields

Value and Option level fields were validated following these docs: https://datadoghq.dev/integrations-core/meta/config-specs/#example-file-consumer

Motivation

https://datadoghq.atlassian.net/browse/AI-5146

The issue was that this spec.yaml:

    - name: tenant
      required: false
      description: |
        A comma-separated list of tenant names, by default set to admin.
      value:
        type: string
        example: "admin,tenant_a,tenant_b"
        default: admin

generated this conf.yaml.example:

    ## @param tenant - string - optional - default: admin,tenant_a,tenant_b
    ## A comma-separated list of tenant names, by default set to admin.
    #
    # tenant: admin,tenant_a,tenant_b

Instead of using "admin" as default, it used "example".
It seemed like it was skipping 'default' in this method:

The goal of this change was so that we could detect fields that did not exist for the option or value level attributes and notify the user when running ddev validate config.

Also, as shown in the example, the default field was not being respected even though it’s a valid field. This change ensures that default will now be properly respected.

Review checklist (to be filled by reviewers)

• Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
• Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
• If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

[ddog-nasirthomas]

I wasn't sure if the ALLOWED_OPTION_FIELDS would be better suited in datadog_checks.dev.tooling.configuration.constants since the file seemed specific to Open API constants. Please let me know if I should move it. Thanks

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Keep snowflake proxy option values as mappings

The proxy_port option now defines value: <PROXY_PORT> at the option level before the nested value: block. Because write_option assigns option['value'] to this string and then expects a mapping (value['type'] and value_type_string(value)), rendering the example or running ddev validate config will fail with a type error (str is not subscriptable) and the proxy settings cannot be documented. The same structure is duplicated for proxy_user and proxy_password below.

Useful? React with 👍 / 👎.

[AAraKKe]

Thanks @ddog-nasirthomas! I didn't finish reviewing everything because I found that some of the changes would actually break validations. You haven't noticed because this branch does no include the latest changes in master and breaks validations soon.

Can you update your branch with the most recent changes, check my comments and update any validation that fails?

Thanks!

My Feedback Legend

Here's a quick guide to the prefixes I use in my comments:

praise: no action needed, just celebrate!
note: just a comment/information, no need to take any action.
question: I need clarification or I'm seeking to understand your approach.
nit: A minor, non-blocking issue (e.g., style, typo). Feel free to ignore.
suggestion: I'm proposing an improvement. This is optional but recommended.
request: A change I believe is necessary before this can be merged.

The only blocking comments are request, any other type of comment can be applied at discretion of the developer.

[ddog-nasirthomas]

@AAraKKe I updated my branch and made the required changes

[AAraKKe]

Thanks a lot @ddog-nasirthomas ! Some last comments but it looks pretty good to go.

[AAraKKe]

question: I am not sure I understand this statement in the elif block. If we make it here it means that neither display_default nor default is in value as we have defined default before as value.get('display_default', value.get('default')). If display_default is not in value, we take the value of default. If default is not there either then it will return None.

Since the previous block only executes when default is not None, it means that one of the 2 previous options is defined. This conditional here says: if display_default is not in value or default is in value then do this. But since display_default will never be en value, or it would have gone to the previous block, this is always true right?

Shouldn't this be just an else as it was before?

[ddog-nasirthomas]

There was a weird issue I encountered when I left it as else and display_default was null, it wouldn't detect it and would result in files being formatted improperly by either trying to set the default or the example in the header.

I believe this is because of the line default = value.get('display_default', value.get('default')) .

If the key exists and the value is set to null, .get() returns None and this would cause the errors.