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

Put mod_vcard config options in a map #3553

Merged
merged 6 commits into from
Feb 23, 2022
Merged

Put mod_vcard config options in a map #3553

merged 6 commits into from
Feb 23, 2022

Conversation

gustawlippa
Copy link
Contributor

@gustawlippa gustawlippa commented Feb 21, 2022

mod_vcard opts put in maps with defaults. LDAP options should probably be reworked after all modules that use them will have maps and sane defaults. Some small fixes to documentation and code.

@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #3553 (b1ee098) into master (d23ac7a) will increase coverage by 0.00%.
The diff coverage is 95.45%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3553   +/-   ##
=======================================
  Coverage   79.23%   79.24%           
=======================================
  Files         420      420           
  Lines       32263    32256    -7     
=======================================
- Hits        25564    25560    -4     
+ Misses       6699     6696    -3     
Impacted Files Coverage Δ
src/vcard/mod_vcard_mnesia.erl 80.45% <66.66%> (ø)
src/vcard/mod_vcard.erl 81.06% <100.00%> (+0.83%) ⬆️
src/vcard/mod_vcard_ldap.erl 93.37% <100.00%> (+1.81%) ⬆️
src/vcard/mod_vcard_riak.erl 87.09% <100.00%> (-1.62%) ⬇️
src/logger/mongoose_log_filter.erl 78.08% <0.00%> (-1.37%) ⬇️
src/ejabberd_c2s.erl 88.67% <0.00%> (-0.30%) ⬇️
src/mod_muc_log.erl 78.11% <0.00%> (ø)
src/pubsub/mod_pubsub.erl 73.31% <0.00%> (+0.11%) ⬆️
src/domain/mongoose_domain_loader.erl 90.17% <0.00%> (+0.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d23ac7a...b1ee098. Read the comment docs.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@gustawlippa gustawlippa force-pushed the mod_vcard-map branch 2 times, most recently from 82e7f66 to d67591f Compare February 21, 2022 14:50
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Feb 21, 2022

small_tests_24 / small_tests / 01d77d7
Reports root / small


small_tests_23 / small_tests / 01d77d7
Reports root / small


dynamic_domains_mysql_redis_24 / mysql_redis / 01d77d7
Reports root/ big
OK: 2704 / Failed: 0 / User-skipped: 256 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 01d77d7
Reports root/ big
OK: 2721 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 01d77d7
Reports root/ big
OK: 2721 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 01d77d7
Reports root/ big
OK: 2721 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 01d77d7
Reports root/ big
OK: 1510 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / 01d77d7
Reports root/ big
OK: 1510 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / 01d77d7
Reports root/ big
OK: 1551 / Failed: 0 / User-skipped: 349 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / 01d77d7
Reports root/ big
OK: 3095 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 01d77d7
Reports root/ big
OK: 1685 / Failed: 0 / User-skipped: 369 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / 01d77d7
Reports root/ big
OK: 3090 / Failed: 0 / User-skipped: 253 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 01d77d7
Reports root/ big
OK: 3095 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / 01d77d7
Reports root/ big
OK: 3095 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 01d77d7
Reports root/ big
OK: 1690 / Failed: 0 / User-skipped: 364 / Auto-skipped: 0

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@gustawlippa gustawlippa marked this pull request as ready for review February 22, 2022 11:01
The values are used only when the Riak backend is specified.
@mongoose-im
Copy link
Collaborator

mongoose-im commented Feb 22, 2022

small_tests_24 / small_tests / b1ee098
Reports root / small


small_tests_23 / small_tests / b1ee098
Reports root / small


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / b1ee098
Reports root/ big
OK: 2721 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / b1ee098
Reports root/ big
OK: 2704 / Failed: 0 / User-skipped: 256 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / b1ee098
Reports root/ big
OK: 2721 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / b1ee098
Reports root/ big
OK: 2721 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / b1ee098
Reports root/ big
OK: 1510 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / b1ee098
Reports root/ big
OK: 1510 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / b1ee098
Reports root/ big
OK: 1551 / Failed: 0 / User-skipped: 349 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / b1ee098
Reports root/ big
OK: 1685 / Failed: 0 / User-skipped: 369 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / b1ee098
Reports root/ big
OK: 3095 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / b1ee098
Reports root/ big
OK: 3095 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / b1ee098
Reports root/ big
OK: 3090 / Failed: 0 / User-skipped: 253 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / b1ee098
Reports root/ big
OK: 3095 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / b1ee098
Reports root/ big
OK: 1690 / Failed: 0 / User-skipped: 364 / Auto-skipped: 0

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Looks good, I think that we could rework some LDAP fields later (if we need to). I only added one suggestion, that we might utilize in the future.

test/common/config_parser_helper.erl Show resolved Hide resolved
Comment on lines +245 to +251
<<"ldap_pool_tag">> => default,
<<"ldap_uids">> => [{<<"uid">>, <<"%u">>}],
<<"ldap_vcard_map">> => mod_vcard_ldap:default_vcard_map(),
<<"ldap_search_fields">> => mod_vcard_ldap:default_search_fields(),
<<"ldap_search_reported">> => mod_vcard_ldap:default_search_reported(),
<<"ldap_search_operator">> => 'and',
<<"ldap_binary_search_fields">> => []
Copy link
Member

Choose a reason for hiding this comment

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

Just a note: how about putting all these options in a nested map ldap and treating them like riak? I know it's a change in config structure, but for me it looks like a change for the better? It could be done in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about it, and even some LDAP options are shared between different modules using LDAP. There could be a section defined in the mongoose_ldap_config with the common options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the change, the options that the user would specify would be, for example modules.mod_vcard.ldap.pool_tag instead of modules.mod_vcard.ldap_pool_tag, correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we could do it separately, in MAM I could also move the options into subsections, but it would be too much of a change in one go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'd rather do it separately as well

@chrzaszcz chrzaszcz merged commit 028f9e4 into master Feb 23, 2022
@chrzaszcz chrzaszcz deleted the mod_vcard-map branch February 23, 2022 09:10
@Premwoik Premwoik added this to the 5.1.0 milestone May 25, 2022
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.

None yet

4 participants