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

feat: changes to support variadic keys for deletion from API dest. #2457

Merged
merged 6 commits into from
Sep 21, 2022

Conversation

saurav-malani
Copy link
Contributor

Description

Currently, when we sent request to transformer to further call down stream destination to perform deletion, the userAttributes section of the request can only have userID (required), phone (optional) & email (optional). We are modifying it to support variadic keys with only userID as required field.

Notion Ticket

https://www.notion.so/rudderstacks/data-regulation-worker-support-key-value-user-attributes-1771ea8b38ac43d1a09e6ada68f2ce83

@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Base: 38.67% // Head: 38.62% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (3925c23) compared to base (082b9f5).
Patch coverage: 96.15% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2457      +/-   ##
==========================================
- Coverage   38.67%   38.62%   -0.06%     
==========================================
  Files         166      166              
  Lines       36683    36670      -13     
==========================================
- Hits        14186    14162      -24     
- Misses      21580    21592      +12     
+ Partials      917      916       -1     
Impacted Files Coverage Δ
...gulation-worker/internal/delete/kvstore/kvstore.go 77.77% <66.66%> (ø)
regulation-worker/internal/client/client.go 67.85% <100.00%> (+0.23%) ⬆️
regulation-worker/internal/delete/api/api.go 74.71% <100.00%> (-0.57%) ⬇️
regulation-worker/internal/delete/batch/batch.go 47.78% <100.00%> (-1.43%) ⬇️
processor/processor.go 71.31% <0.00%> (-0.78%) ⬇️
config/backend-config/backend-config.go 69.88% <0.00%> (+0.56%) ⬆️
config/backend-config/namespace_config.go 78.62% <0.00%> (+2.29%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@saurav-malani saurav-malani marked this pull request as ready for review September 20, 2022 10:58
@atzoum
Copy link
Contributor

atzoum commented Sep 21, 2022

Are we going to include this in a patch release or it will be simply included in the next scheduled release?

@cisse21
Copy link
Member

cisse21 commented Sep 21, 2022

We can patch this and make a release for the specific customer who needs it

@atzoum
Copy link
Contributor

atzoum commented Sep 21, 2022

We can patch this and make a release for the specific customer who needs it

A patch release for a feature is not something we can really do. Our options as I see it are:

  1. Mask it as a fix and change the target branch to release/1.1.x. Follow the patch release process (1.1.2) from there on.
  2. Merge it normally in the main branch and cherry-pick it in a customer-specific branch (e.g. 1.1.1-<customername>-hotfix.1) on top of tag 1.1.1,
    The first approach doesn't really make much sense to me

@saurav-malani saurav-malani force-pushed the feat.addVariadicKeySupportForApiDest branch from 9f525ae to 3925c23 Compare September 21, 2022 11:17
@lvrach
Copy link
Member

lvrach commented Sep 21, 2022

I would go with our normal flow. If it is urgent and since the worker is building as a separate binary and runs on a separate pod, we can deploy the master tag to the worker pod of the specific customer

@saurav-malani saurav-malani merged commit 1950713 into master Sep 21, 2022
@saurav-malani saurav-malani deleted the feat.addVariadicKeySupportForApiDest branch September 21, 2022 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants