Release R141 #1258
Release R141 #1258
Conversation
|
Hi @matus-tomlein! I have two PRs (#1284 #1286) that are approved already. Would it be okay if I merge them to this PR to include them to the next release ? |
|
Thanks @matus-tomlein! I've merged the schemas. @igneel64 Please let me know if you see any problem with merging them. |
|
Hey @spenes :) As I checked, the schemas have been approved in their respective PRs so I see no problems from my side. Now it can be up to @jbeemster on when the deployment will happen. |
| } | ||
| }, | ||
| "additionalProperties": false, | ||
| "required": ["inputs", "database", "query", "output", "cache", "ignoreOnError"] |
There was a problem hiding this comment.
@istreeter I always thought that (at least for Redshift) that you could not "add" new required fields and that this was a breaking change for the table. This was ingrained into me a long time ago so could well be outdated now and is likely not relevant for this schema given its being used to configure an enrichment and won't ever become a table in reality.
I would have thought that a new required field needs a major version bump generally?
There was a problem hiding this comment.
The Redshift loader will now handle this gracefully, so technically there is nothing to worry about.
But....
You are correct, we shouldn't add a new field as a required field. Because it breaks the principles of using schema ver.
The guiding principle of schema versioning is: The latest version of a schema must describe all historic data. In this case, json that was valid against the 1-0-0 schema is not valid against the 1-0-1 schema, because it would be missing the ignoreOnError field.
I think it would be better to make ignoreOnError an optional field. And I don't think that causes any problems for how we want to use this schema.
cc @pondzix
There was a problem hiding this comment.
The same issues is in the API enrichment bump also!
There was a problem hiding this comment.
Ok, I made ignorOnError optional for both: SQL and API enrichments
matus-tomlein
force-pushed
the
release/r141
branch
from
cb77fbc
to
f10d2b4
Compare
|
Sorry for messing with this release last minute, but we just got the go ahead from @istreeter to release a minor version of the It has been previously reviewed in R140, but based on this comment from Ian we postponed the release. |
| "required": ["size", "ttl"] | ||
| }, | ||
| "ignoreOnError": { | ||
| "type": "boolean" |
There was a problem hiding this comment.
Not to nitpick - should this be nullable if this is not required?
There was a problem hiding this comment.
So now we could omit ignoreOnError but not set explicitly null. With "type": ["boolean", "null"] we could omit and also set null. I can't see the reason why we shoudn't do that, so...I will add null ;)
This PR adds the
refundschema for the snowplow ecommerce tracking.