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.

Sign up for GitHub

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

Jump to bottom

call filters: autogenerate name for multi-tenancy purposes #401

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

call filters: autogenerate name for multi-tenancy purposes #401

wants to merge 11 commits into from

Conversation

bloom1
Copy link
Member

@bloom1 bloom1 commented Jul 5, 2023 •
edited
Loading

@bloom1 bloom1 force-pushed the WAZO-3313-multi-tenant-call-filter branch 3 times, most recently from 1c854f8 to 35f3184 Compare July 5, 2023 18:21
@wazo-community-zuul
Copy link
Contributor

This change depends on a change that failed to merge.

@bloom1
Copy link
Member Author

bloom1 commented Jul 5, 2023

recheck

@wazo-community-zuul
Copy link
Contributor

Build failed.

@wazo-community-zuul
Copy link
Contributor

Build failed.

@wazo-community-zuul
Copy link
Contributor

Build failed.

@bloom1 bloom1 force-pushed the WAZO-3313-multi-tenant-call-filter branch from b9d6385 to 44eb037 Compare July 5, 2023 20:43
@wazo-community-zuul
Copy link
Contributor

Build failed.

@wazo-community-zuul
Copy link
Contributor

Build failed.

@wazo-community-zuul
Copy link
Contributor

Build succeeded.

@bloom1 bloom1 added the mergeit label Jul 6, 2023
@bloom1 bloom1 force-pushed the WAZO-3313-multi-tenant-call-filter branch from 2b1d8b8 to 60775c9 Compare July 10, 2023 13:30
@wazo-community-zuul
Copy link
Contributor

Build succeeded.

@bloom1 bloom1 force-pushed the WAZO-3313-multi-tenant-call-filter branch from 60775c9 to 3f4ac46 Compare July 11, 2023 20:53
@wazo-community-zuul
Copy link
Contributor

Build failed.

@bloom1
Copy link
Member Author

bloom1 commented Jul 12, 2023

recheck

@wazo-community-zuul
Copy link
Contributor

Build failed.

sduthil
sduthil reviewed Jul 17, 2023
View reviewed changes
wazo_confd/plugins/call_filter/resource.py Outdated
Comment on lines 33 to 34
# NOTE(afournier): we use a UUID as if it was the callfilter UUID but it's not
# Call filters do not use UUIDs yet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be hard to add the UUID column right now? This would avoid a painful migration (i.e. parsing names to get the UUID) later on when we do add the UUID column.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's another ticket completely. Call filters are far from being the only resource still using an integer ID.
It's also hard, requiring changes in function keys, fallbacks, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how about adding the column uuid but keeping the id as primary key, only ensuring that the uuid is unique? That way we have the uuid in the database, we can use it for the callfilter name and the primary key migration id -> uuid will be easier.

@bloom1 bloom1 force-pushed the WAZO-3313-multi-tenant-call-filter branch from 3f4ac46 to 5dbc2a0 Compare July 17, 2023 14:11
@wazo-community-zuul
Copy link
Contributor

Build succeeded.

sduthil
sduthil reviewed Jul 18, 2023
View reviewed changes
integration_tests/suite/base/test_call_filters.py
Comment on lines -41 to -45
yield s.check_bogus_field_returns_error, url, 'name', 123
yield s.check_bogus_field_returns_error, url, 'name', None
yield s.check_bogus_field_returns_error, url, 'name', True
yield s.check_bogus_field_returns_error, url, 'name', {}
yield s.check_bogus_field_returns_error, url, 'name', []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any test covering the deprecated way by sending only a name? We still want to support that use case and it needs at least 2 tests (one for success, one for error/invalid data)

@bloom1 bloom1 force-pushed the WAZO-3313-multi-tenant-call-filter branch from 5dbc2a0 to 2312375 Compare July 18, 2023 18:18
@wazo-community-zuul
Copy link
Contributor

Build failed.

@bloom1 bloom1 force-pushed the WAZO-3313-multi-tenant-call-filter branch from 2312375 to e064f14 Compare July 18, 2023 18:57
@bloom1 bloom1 force-pushed the WAZO-3313-multi-tenant-call-filter branch from e064f14 to e53ede0 Compare July 18, 2023 18:58
@sonarcloud
Copy link

sonarcloud bot commented Jul 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@wazo-community-zuul
Copy link
Contributor

Build failed.

@bloom1
Copy link
Member Author

bloom1 commented Jul 18, 2023

recheck

@wazo-community-zuul
Copy link
Contributor

Build succeeded.

This was referenced Jul 25, 2023
Merged
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sduthil sduthil sduthil left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
mergeit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bloom1 @sduthil