-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: configurable extra allowable metadata field attributes #73
base: master
Are you sure you want to change the base?
Conversation
97af4f8
to
fefefa3
Compare
fefefa3
to
7023531
Compare
|
||
# Define extra allowable metadata attributes for fields | ||
def self.extra_metadata_attributes(*attrs) | ||
Avro::Builder::Field.extra_metadata_attributes(attrs) |
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.
It looks like the spec supports metadata attributes on all JSON object in the serialized Avro schema. Is there a generalized version of this change that supports attributes on constructs other than just fields e.g. records?
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.
@jturkel Yes, I can make that change no problem. Would you rather seperate config blocks for declaring the attributes for for each object type? Or is all together ok?
Avro::Builder.extra_metadata_attributes(:sensitivity_level, :deprecated_by)
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.
I'm not familiar with how people are using metadata on Avro schemas but my instinct is to keep it simple for now with a single set of attributes that applies to all types/fields. We can always extend this in the future to support both global and type specific metadata attributes.
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.
@jturkel Fantastic. I’ve pushed that change. There is the possibility still to have individual setters for records vs fields in the future if you want too.
fd542fc
to
dac057d
Compare
dac057d
to
5f3eca6
Compare
# Define extra allowable metadata attributes for fields | ||
def self.extra_metadata_attributes(*attrs) | ||
Avro::Builder::Field.extra_metadata_attributes(attrs) | ||
Avro::Builder::Record.extra_metadata_attributes(attrs) |
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.
It looks like the Avro spec supports metadata on all types rather than just record types. If that's correct, then should this be on Avro::Builder::Types::Type
rather than Avro::Builder::Types::RecordType
?
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.
@jturkel sorry I haven’t got back to this because I hit some code here that could benefit from a refactor. Any possibility we could merge this as is and then circle back later?
According to the Avro specification any attribute can be added to a field and used as extra metadata provided it does not clash with the other attributes specified.
To enable the use of specific custom metadata attributes you can define them with:
I also solved #9 when adding this.