-
Notifications
You must be signed in to change notification settings - Fork 114
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
Release R141 #1258
Release R141 #1258
Conversation
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.
Has already been reviewed for #1256 , so I suppose everything is ok!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same issues is in the API enrichment bump also!
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.
Ok, I made ignorOnError
optional for both: SQL and API enrichments
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to nitpick - should this be nullable if this is not 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.
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
refund
schema for the snowplow ecommerce tracking.