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.

Sign up for GitHub

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

Jump to bottom

Generate schema for pipeline.yaml and config.yaml #70

Merged
merged 45 commits into from
Jan 30, 2023
Merged

Generate schema for pipeline.yaml and config.yaml #70

merged 45 commits into from
Jan 30, 2023

Conversation

disrupted
Copy link
Member

@disrupted disrupted commented Jan 26, 2023 •
edited
Loading

Closes #14
Supersedes #67

@disrupted
Refactor to make internal fields private
9a8e3c0
@disrupted
Generate schema
a463ad7
@disrupted
WIP
1322ff3
@disrupted
Revert "WIP"
b2c948b 
This reverts commit 1322ff3.
@disrupted
Revert "Refactor to make internal fields private"
96b4a09 
This reverts commit 9a8e3c0.
@disrupted
Fix usage of private attr
2c30f48
@disrupted
Fix static class variable
448cc68
@disrupted
Remove class var
e5d5c51
@disrupted
Fix test
dd6730a
@disrupted
Revert "Remove class var"
455fb8f 
This reverts commit e5d5c51.
@disrupted
Workaround Pydantic discriminator
89cfeb3
@disrupted
Exclude config & enrich from schema
09550be
@disrupted
Fix issues
7c9aeb7
@disrupted
Add property
370a28d
@disrupted
Update config & handlers attrs
9c7b8eb
@disrupted
Rename schema codegen
54be760
@disrupted
Work around Pydantic quirks
1d5b5ca
@disrupted
Work around Pydantic quirks
26281f2
@disrupted
Exclude schema_type from generate output
cc6b4a9
@disrupted
Fix producer tests
c86ac79
@disrupted
Fix test
c5a2a8e
@disrupted
Use field init instead of kwargs
1a8737f
@disrupted
Fix pydantic workaround
5949b99
@disrupted disrupted self-assigned this Jan 26, 2023
raminqaf
raminqaf reviewed Jan 26, 2023
View reviewed changes
gen_schema.py Outdated Show resolved Hide resolved
@raminqaf
Copy link
Contributor

what is the reason behind all these conflicts!? 😱

@disrupted
Copy link
Member Author

what is the reason behind all these conflicts!? 😱

I've been working from an older base, need to merge main first

@disrupted disrupted marked this pull request as draft January 26, 2023 14:56
@disrupted disrupted changed the title Generate schema Generate pipeline schema Jan 26, 2023
@disrupted disrupted marked this pull request as ready for review January 26, 2023 16:04
sujuka99
sujuka99 previously approved these changes Jan 27, 2023
View reviewed changes
Copy link
Contributor

@sujuka99 sujuka99 left a comment

Choose a reason for hiding this comment

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

Looks great!

Just a small comment, if you generate a schema from PipelineConfig, we could have autocompletion for config.yaml as well. It was much simpler and worked out of the box. I did it by accident while exploring.

Simply {PipelineConfig.schema_json(indent=2) generated pipeline_config.json in this PR.

I see no downsides of having auto completion for that file as well, but I don't know if it should be added in this PR or a subsequent one.

raminqaf
raminqaf reviewed Jan 30, 2023
View reviewed changes
Copy link
Contributor

@raminqaf raminqaf left a comment

Choose a reason for hiding this comment

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

LGTM! I had some notices and questions!

gen_schema.py Show resolved Hide resolved
gen_schema.py Show resolved Hide resolved
gen_schema.py Outdated Show resolved Hide resolved
kpops/components/base_components/base_defaults_component.py Show resolved Hide resolved
kpops/components/streams_bootstrap/producer/producer_app.py Outdated Show resolved Hide resolved
kpops/components/streams_bootstrap/streams/streams_app.py Outdated Show resolved Hide resolved
sujuka99
sujuka99 previously approved these changes Jan 30, 2023
View reviewed changes
raminqaf
raminqaf previously approved these changes Jan 30, 2023
View reviewed changes
@disrupted disrupted dismissed stale reviews from raminqaf and sujuka99 via 4e78163 January 30, 2023 09:37
raminqaf
raminqaf previously approved these changes Jan 30, 2023
View reviewed changes
@disrupted
Copy link
Member Author

Just a small comment, if you generate a schema from PipelineConfig, we could have autocompletion for config.yaml as well. It was much simpler and worked out of the box. I did it by accident while exploring.

Simply {PipelineConfig.schema_json(indent=2) generated pipeline_config.json in this PR.

I see no downsides of having auto completion for that file as well, but I don't know if it should be added in this PR or a subsequent one.

Good point! I've added it.

sujuka99
sujuka99 approved these changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@sujuka99 sujuka99 left a comment

Choose a reason for hiding this comment

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

LGTM

@disrupted disrupted changed the title Generate pipeline schema Generate schema for pipeline.yaml and config.yaml Jan 30, 2023
@disrupted disrupted merged commit 7bb1345 into main Jan 30, 2023
@disrupted disrupted deleted the feat/schema branch January 30, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sujuka99 sujuka99 sujuka99 approved these changes

@raminqaf raminqaf raminqaf left review comments

Assignees

@disrupted disrupted

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide JSON schema for pipeline yaml
3 participants
@disrupted @raminqaf @sujuka99