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.

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

Add com.snowplowanalytics.snowplow/button_click/jsonschema/1-0-1 #1428

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jack-Keene
Copy link

@Jack-Keene Jack-Keene commented Feb 28, 2025

This PR makes the label property of the button_click schema an optional property. This is to handle buttons without any inner text - for example icon buttons.

I've also added the $supersedes property for the previous v1-0-0 schema

@istreeter
Copy link
Contributor

Hi @Jack-Keene regarding $supersedes, please refer to this conversation in a previous PR. We concluded that in Iglu Central you need to add $supersedes to the new schema AND $supersededBy to the old schema.

After you've read that, then please refer to this follow-up conversation where it got decided simply to remove the required from the previous schema, instead of releasing a new schema.

I think the situation for button_click is the same as the situation for client_session which got discussed in the earlier conversations.

@matus-tomlein
Copy link
Contributor

matus-tomlein commented Mar 4, 2025

Sorry missed this PR. Agree with @istreeter, but just want to add that when in the client_session case we replaced the schema, a customer complained that it "broke a bunch of events for them". I don't really understand how, @johnmicahreid followed up with them to get more info. But I think it might be better to avoid the replace and just release a new patch version that we update in our trackers. It's not as neat as customers will need to update their tracking, but probably safer.

@istreeter
Copy link
Contributor

Oh... so that might be because in Redshift it is not possible to remove a not null constraint from an existing column. In which case this change needs to be a 2-0-0 schema, not a 1-0-1 schema.

@matus-tomlein
Copy link
Contributor

hmm... @johnmicahreid do we still want the change (make the label optional) if it requires a new major version of the schema (so a new column in the events table or a new table in redshift which could be breaking for downstream applications)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants