- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.9k
fix(metadata models): correct SecretStore required fields and BreakingChanges const usage #68670
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(metadata models): correct SecretStore required fields and BreakingChanges const usage #68670
Conversation
…erences - Change SecretStore required fields from [name, secretStore] to [type, alias] - These were incorrectly copied from Secret.yaml - Verified all 304 connectors with secretStore have both type and alias - Fix SecretStore.yaml to point to SecretStore.yaml instead of TestSecret.yaml - Fix Secret.yaml to point to Secret.yaml instead of TestSecret.yaml Co-Authored-By: AJ Steers <aj@airbyte.io>
… schema - Replace invalid 'type: const' with 'type: string' alongside 'const: stream' - JSON Schema Draft 7 does not support 'const' as a type value - The const keyword is a validation keyword, not a type - This was causing validation to fail with 'Unknown type const' error Co-Authored-By: AJ Steers <aj@airbyte.io>
| Original prompt from AJ Steers | 
| 🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically: 
 Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options: 
 | 
| 👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Helpful Resources
 PR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR: 
 | 
| title: SecretStore | ||
| description: An object describing a secret store metadata | ||
| type: object | ||
| required: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Devin, the affect of them being missed was that neither was actually required in the pydantic models. Let's make neither of them required here either.
The previous required fields (name, secretStore) were invalid and pointed to non-existent properties. This meant the generated Pydantic models effectively treated all fields as optional. Making both type and alias explicitly optional maintains backward compatibility with existing behavior. All 304 connectors with secretStore entries already include both type and alias fields, so this change is purely for backward compatibility and won't mask any existing data quality issues. Co-Authored-By: AJ Steers <aj@airbyte.io>
        
          
                airbyte-ci/connectors/metadata_service/lib/metadata_service/models/src/SecretStore.yaml
          
            Show resolved
            Hide resolved
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking comment
What
Fixes critical bugs in metadata service model source YAML files that were preventing validation of connector metadata.yaml files. These bugs were discovered during validation testing for airbyte-python-cdk PR 807 which adds JSON schema validation for metadata.yaml files.
Issues fixed:
type: constsyntax in ConnectorBreakingChanges.yaml (JSON Schema Draft 7 does not supportconstas a type value)$idreferences (both Secret.yaml and SecretStore.yaml pointed to non-existent TestSecret.yaml)How
ConnectorBreakingChanges.yaml
type: constwithtype: stringalongsideconst: streamconstas a validation keyword, not a typeSecretStore.yaml
required: [name, secretStore]torequired: [type, alias]type(enum: ["GSM"]) andalias(string)typeandaliasfields$idto point to SecretStore.yaml instead of TestSecret.yamlSecret.yaml
$idto point to Secret.yaml instead of TestSecret.yamlReview guide
type: string+const: streamis valid JSON Schema Draft 7 syntaxtypeandalias(notnameandsecretStore) by checking actual connector metadata.yaml filesNote: These source YAML files are combined into a JSON schema in airbyte-python-cdk. After this PR merges, the schema will need to be regenerated in PR 807 for the fixes to take effect.
User Impact
Before: Validation of connector metadata.yaml files failed for all 10 tested certified connectors due to schema bugs
After: Once schema is regenerated in airbyte-python-cdk, metadata.yaml validation will work correctly
No immediate user impact - these are source schema definition files. Changes only take effect after regeneration in the CDK repo.
Can this PR be safely reverted and rolled back?
These are source YAML files that need regeneration to take effect. Reverting would simply restore the buggy state before any downstream regeneration occurs.
Link to Devin run: https://app.devin.ai/sessions/0078df9742174c169753b2c4c4d247da
Requested by: AJ Steers (@aaronsteers)