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

Use maps for mod_keystore config #3599

Merged
merged 2 commits into from
Mar 21, 2022
Merged

Use maps for mod_keystore config #3599

merged 2 commits into from
Mar 21, 2022

Conversation

gustawlippa
Copy link
Contributor

The validation of different keys has been moved to parsing the config. Thus, the test is also moved to the config_parser_SUITE.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 18, 2022

small_tests_24 / small_tests / af191c9
Reports root / small


small_tests_23 / small_tests / af191c9
Reports root / small


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / af191c9
Reports root/ big
OK: 2822 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / af191c9
Reports root/ big
OK: 2805 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / af191c9
Reports root/ big
OK: 2822 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / af191c9
Reports root/ big
OK: 1505 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / af191c9
Reports root/ big
OK: 1505 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / af191c9
Reports root/ big
OK: 2822 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / af191c9
Reports root/ big
OK: 1546 / Failed: 0 / User-skipped: 358 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / af191c9
Reports root/ big
OK: 3191 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / af191c9
Reports root/ big
OK: 3196 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / af191c9
Reports root/ big
OK: 3196 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / af191c9
Reports root/ big
OK: 1853 / Failed: 0 / User-skipped: 366 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / af191c9
Reports root/ big
OK: 3196 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / af191c9
Reports root/ big
OK: 1696 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0

@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #3599 (d6b2f35) into master (b430804) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##           master    #3599   +/-   ##
=======================================
  Coverage   80.88%   80.89%           
=======================================
  Files         426      426           
  Lines       32249    32236   -13     
=======================================
- Hits        26086    26077    -9     
+ Misses       6163     6159    -4     
Impacted Files Coverage Δ
src/mod_keystore.erl 88.09% <80.00%> (-1.00%) ⬇️
..._distrib/mod_global_distrib_outgoing_conns_sup.erl 80.00% <0.00%> (-8.00%) ⬇️
src/ejabberd_sm.erl 84.59% <0.00%> (-0.33%) ⬇️
src/mod_muc_log.erl 63.21% <0.00%> (ø)
src/mod_muc_room.erl 76.70% <0.00%> (+0.11%) ⬆️
...c/global_distrib/mod_global_distrib_server_mgr.erl 77.71% <0.00%> (+0.57%) ⬆️
src/logger/mongoose_log_filter.erl 79.45% <0.00%> (+1.36%) ⬆️
src/inbox/mod_inbox_rdbms_async.erl 70.68% <0.00%> (+3.44%) ⬆️

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 b430804...d6b2f35. Read the comment docs.

Because the validation of different keys has been moved to parsing the config,
the test is also moved to the config_parser_SUITE.
@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 18, 2022

small_tests_24 / small_tests / b56b254
Reports root / small


small_tests_23 / small_tests / b56b254
Reports root / small


dynamic_domains_mysql_redis_24 / mysql_redis / b56b254
Reports root/ big
OK: 2805 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / b56b254
Reports root/ big
OK: 2822 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / b56b254
Reports root/ big
OK: 2822 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / b56b254
Reports root/ big
OK: 2822 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / b56b254
Reports root/ big
OK: 1505 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / b56b254
Reports root/ big
OK: 1505 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / b56b254
Reports root/ big
OK: 1546 / Failed: 0 / User-skipped: 358 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / b56b254
Reports root/ big
OK: 3196 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / b56b254
Reports root/ big
OK: 3196 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / b56b254
Reports root/ big
OK: 3203 / Failed: 1 / User-skipped: 147 / Auto-skipped: 0

muc_SUITE:hibernation:hibernated_room_can_be_queried_for_archive
{error,{{assertion_failed,assert,is_groupchat_message,
              [<<"Restorable message">>],
              undefined,"undefined"},
    [{escalus_new_assert,assert_true,2,
               [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
                {line,84}]},
     {muc_SUITE,wait_for_mam_result,3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4383}]},
     {muc_SUITE,'-hibernated_room_can_be_queried_for_archive/1-fun-0-',3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4124}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {muc_SUITE,hibernated_room_can_be_queried_for_archive,1,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4120}]},
     {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
     {test_server,run_test_case_eval1,6,
            [{file,"test_server.erl"},{line,1292}]},
     {test_server,run_test_case_eval,9,
            [{file,"test_server.erl"},{line,1224}]}]}}

Report log


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / b56b254
Reports root/ big
OK: 1853 / Failed: 0 / User-skipped: 366 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / b56b254
Reports root/ big
OK: 3196 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / b56b254
Reports root/ big
OK: 1696 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0

@gustawlippa gustawlippa marked this pull request as ready for review March 18, 2022 15:53
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.

I have a suggestion. What do you think about it?

@@ -84,7 +78,11 @@ config_spec() ->
items = #{<<"ram_key_size">> => #option{type = integer,
validate = non_negative},
<<"keys">> => #list{items = keys_spec()}
Copy link
Member

Choose a reason for hiding this comment

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

We could validate the keys here by just adding format_items = map and thus checking the uniqueness without the need for the whole validate_key_ids function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this fail, or just take the last key? I assumed it would work without failure (while we would like an error to be thrown).

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thanks for the clarification - done.

src/mod_keystore.erl Show resolved Hide resolved
@mongoose-im

This comment was marked as outdated.

@gustawlippa gustawlippa force-pushed the mod_keystore-map-config branch 2 times, most recently from 8aaec66 to 40a2e23 Compare March 21, 2022 09:31
@mongoose-im

This comment was marked as outdated.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 21, 2022

small_tests_23 / small_tests / d6b2f35
Reports root / small


small_tests_24 / small_tests / d6b2f35
Reports root / small


dynamic_domains_mysql_redis_24 / mysql_redis / d6b2f35
Reports root/ big
OK: 2805 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / d6b2f35
Reports root/ big
OK: 2822 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / d6b2f35
Reports root/ big
OK: 2822 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / d6b2f35
Reports root/ big
OK: 2822 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / d6b2f35
Reports root/ big
OK: 1505 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / d6b2f35
Reports root/ big
OK: 1505 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / d6b2f35
Reports root/ big
OK: 1546 / Failed: 0 / User-skipped: 358 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / d6b2f35
Reports root/ big
OK: 3196 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / d6b2f35
Reports root/ big
OK: 3196 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / d6b2f35
Reports root/ big
OK: 1853 / Failed: 0 / User-skipped: 366 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / d6b2f35
Reports root/ big
OK: 3191 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / d6b2f35
Reports root/ big
OK: 3196 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / d6b2f35
Reports root/ big
OK: 1696 / Failed: 0 / User-skipped: 365 / 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 👍

@chrzaszcz chrzaszcz merged commit a9c68bd into master Mar 21, 2022
@chrzaszcz chrzaszcz deleted the mod_keystore-map-config branch March 21, 2022 15:55
@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