Add a dictionary of links to AvailabilityState. #1214
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
agorajek
force-pushed
the
DJ-41
branch
2 times, most recently
from
6dc424b
to
caf66a0
Compare
There was a problem hiding this comment.
This is awesome and is going to really unlock the seamless end to end dev flow we've been working towards. Huge thanks @agorajek. One thing I was wondering is if we should expect cases where someone wants to add links but not necessarily attach it to the materialization for a node. For example if a metric node has a doc that might be useful to link out to. I think it's enough to suggest they just put it in the metric description but just wanted to get your thoughts on that.
| @@ -31,6 +31,7 @@ class AvailabilityState(Base): # pylint: disable=too-few-public-methods | |||
| table: Mapped[str] | |||
| valid_through_ts: Mapped[int] = mapped_column(sa.BigInteger()) | |||
| url: Mapped[Optional[str]] | |||
There was a problem hiding this comment.
I wonder if we can actually remove url now, since that was just a single link, but with this change we'll have the option for multiple, and all named as well. Although there will be a database migration involved if we do remove.
There was a problem hiding this comment.
I was thinking about it. And I noticed that the top level url attribute is typically used as a link to a canonical catalog for a particular dataset, which makes sense to be separate. But then it should be called catalog_url so maybe I should just rename it, if you agree with this idea?
There was a problem hiding this comment.
Actually dataset_url sounds more intuitive than catalog_url. What do you think?
There was a problem hiding this comment.
I'll skip this for now.
I think we will want to have "links" in various places. This one is specific to the Availability State / MaterializedDataset, hence I added it on that entity. In the case of other links I think we should place them as close to the proper entity as possible. Because metric nodes (an all nodes) have a description that's a good place to add a doc link, but I can see having a generic links field on the nodes as well. This way the integrations with other tools are easier to pull off. |
Summary
This feature allows to integrate our AvailabilityStates with external tools, such as data visualizations. For example... when a new availability state is added or updated, we can submit additional urls that can be shown on the Materialization tab of the node.
This PR adds:
linkstogether with the Materialized DatasetsExample:
Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan