-
Notifications
You must be signed in to change notification settings - Fork 72
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
Attentive fix #5319
Attentive fix #5319
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
fides Run #10226
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5319/merge
|
Run status |
Passed #10226
|
Run duration | 00m 39s |
Commit |
3181c55908 ℹ️: Merge dc608c4fdd23dc4686f418a81d61f47b3a1d5903 into a6510c126e548353bfa0c6dcde66...
|
Committer | Bruno Gutierrez Rios |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
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.
It looks like this change has some unwanted change (74 files changed 😬). Can you try fixing or recreating the feature branch to only include the relevant changes?
9ed7770
to
dc90025
Compare
Looks like something happened with git and it decided to include everything on the main branch that was changed on the previous weeks. Rebased and cleaned it up, it should be ok now |
dc90025
to
4a7fe14
Compare
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 change looks good! I tested it locally and the upgrade/downgrade worked as expected. I just updated the down revision to be up to date with main and made some small formatting/label changes in the attentive_config.yml but the rest looks solid
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.
Redacting my approval, it looks like we have some failing tests. Let's fix those before we can merge this in
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.
Thanks for fixing the tests, this looks good to me!
fides Run #10243
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #10243
|
Run duration | 00m 40s |
Commit |
1e347901db: Attentive fix (#5319)
|
Committer | Bruno Gutierrez Rios |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
View all changes introduced in this branch ↗︎ |
Co-authored-by: Adrian Galvan <[email protected]>
Co-authored-by: Adrian Galvan <[email protected]>
Description Of Changes
Currently we have the Attentive email integration as a high level connection type category, and that was causing conflict with lower level connection types, such as the new SaaS Attentive configuration. Since we are transitioning into using SaaS we've updated the reference to attentive_email, in order to avoid name collisions
Ideally, the Attentive Email integration should be a generic email integration. That should be the next step on the transition
Note: We Should check our strategy of communication with our clients before updating this, to avoid any breakage
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md
main
downgrade()
migration is correct and works