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

[KAMELEOON] added type property for every event #1999

Merged

Conversation

akalyuzhnyi
Copy link
Contributor

The Kameleoon cloud destination.

Testing
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

added type property for every event

@joe-ayoub-segment
Copy link
Contributor

Hi @akalyuzhnyi this PR doesn't look right. Didn't we just deploy some new Actions for the Kameleoon Destination via this PR? #1974

Looks like this PR is doing something similiar?

@akalyuzhnyi
Copy link
Contributor Author

akalyuzhnyi commented Apr 17, 2024

Hello @joe-ayoub-segment

Sorry there was wrong MR, now everything is ok

I forgot to add types for the events, could you review please?

@joe-ayoub-segment
Copy link
Contributor

Hi @akalyuzhnyi the change mostly looks good. However you need to be super careful about adding a new 'required' field. Please see comment above.

If you have customers already using this Integration, I would make the type field optional.

@akalyuzhnyi
Copy link
Contributor Author

yes, same type like before for logEvent :)

@joe-ayoub-segment
Copy link
Contributor

yes, same type like before for logEvent :)

hi @akalyuzhnyi - if I deploy the code as it is now it will break any data collection for customers who are already using it.
Is this OK for you?

@akalyuzhnyi
Copy link
Contributor Author

yes, same type like before for logEvent :)

hi @akalyuzhnyi - if I deploy the code as it is now it will break any data collection for customers who are already using it. Is this OK for you?

Ok for me

@joe-ayoub-segment joe-ayoub-segment merged commit d316191 into segmentio:main Apr 23, 2024
10 of 11 checks passed
@joe-ayoub-segment
Copy link
Contributor

note: PR deployed on Tuesday this week

@akalyuzhnyi
Copy link
Contributor Author

note: PR deployed on Tuesday this week

Hello! everything works fine, thank you!

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