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

Settings rework #125

Merged
merged 20 commits into from
Jul 1, 2020
Merged

Settings rework #125

merged 20 commits into from
Jul 1, 2020

Conversation

DenysGonchar
Copy link
Collaborator

@DenysGonchar DenysGonchar commented Jun 23, 2020

This pull request adds the next functionality:

  • configuration of the helper modules in the same way as scenarios (using -required_variable(...) module attribute).
  • explicit overriding of parameters declaration in a scenario. It might be required to change update method for some parameters for some scenarios
  • possibility to update settings using amoc_controller:update_settings() interface. Only if update method is set explicitly for the parameter, otherwise it's read_only
  • changed format of the -required_variable(...) module attribute declaration. the new format is declared here. the old format is still supported.
  • extended testing of amoc_config_* modules.

overview of the amoc_config_* modules:

  • amoc_config - getter module, the only one that is supposed to be used by helper/scenario modules.
  • amoc_config_env - contains functionality for parsing (os) env variables.
  • amoc_config_attributes - parsing/validation of the -required_variable(...) module attributes.
  • amoc_config_validation - verification of the parameters values.
  • amoc_config_scenario - composition of the -required_variable(...) attributes from all the helpers and started scenario, runtime update of the values.
  • amoc_config_utils various helper functions.
  • amoc_scenario - collects a list of all the parametrized helper modules.

open questions:

  • do we want to support an old format of the -required_variable(...) module attributes?
  • do we want to keep config_verification_modules? It's quite complex but I don't know how to deal with it better. currently it works in the next way:
    • if verification/update method is an atom, first we try to find an exported function in the module that declares variable.
    • then we check the modules mentioned in config_verification_modules env of the module's application.
    • then we check the modules mentioned in config_verification_modules env of all other loaded applications.

@DenysGonchar DenysGonchar added the WIP Work In Progress label Jun 23, 2020
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.

Parameterized modules is a very old concept in Erlang: http://erlang.org/workshop/2003/paper/p29-carlsson.pdf

Could you rename this to avoid the clash?

@DenysGonchar DenysGonchar force-pushed the settings-rework branch 3 times, most recently from a2c9f14 to f896248 Compare June 23, 2020 16:58
This reverts commit f3f3997417638030f8a27b46183107bd4edd33d5.
@DenysGonchar DenysGonchar removed the WIP Work In Progress label Jun 25, 2020
@DenysGonchar
Copy link
Collaborator Author

Parameterized modules is a very old concept in Erlang: http://erlang.org/workshop/2003/paper/p29-carlsson.pdf

Could you rename this to avoid the clash?

tmp.erl:

-module(tmp, [Server]).

now try to compile it:

Eshell V10.7.2.1  (abort with ^G)
1> c(tmp).
tmp.erl:2: parameterized modules are no longer supported
error

it's a bit hard to clash with something that is "dead". but if you can suggest some better name for this functionality i will change it.

@chrzaszcz
Copy link
Member

chrzaszcz commented Jun 25, 2020

it's a bit hard to clash with something that is "dead". but if you can suggest some better name for this functionality i will change it.

I know what happened with them, I've been using them back in the old days until they got removed ;-)
I just think that we should not use this term as it is misleading.

Regarding a better name, maybe after reviewing the whole PR I could come up with something, the only idea that comes to my mind is that maybe there is no need to divide modules into regular and parameterized ones, but just use the parameter list as the differentiator (empty or not empty).

@DenysGonchar
Copy link
Collaborator Author

it's a bit hard to clash with something that is "dead". but if you can suggest some better name for this functionality i will change it.

I know what happened with them, I've been using them back in the old days until they got removed ;-)
I just think that we should not use this term as it is misleading.

Do we need to divide the modules into regular and parameterized ones?

yes, we need the list of parametrized modules to prepare the configuration before we start the scenario. the main idea is to do it in one and the same way as scenario configuration.

update can be done like this:
   amoc_controller:update_settings([{interarrival, NewValue}]).
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.

Thanks for the rework, I know there was a need for it.

The code looks good, but I have a few concerns, mostly regarding complexity and clarity of the code. Could you take them into consideration?

integration_test/dummy_scenario.erl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config.hrl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config.hrl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config_attributes.erl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config.hrl Outdated Show resolved Hide resolved
src/amoc_scenario.erl Outdated Show resolved Hide resolved
src/amoc_scenario.erl Outdated Show resolved Hide resolved
src/amoc_scenario.erl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config_validation.erl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config.hrl Show resolved Hide resolved
Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

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

Thanks for the huge effort to make amoc and amoc scenarios configuration consistent and more convenient from the REST API perspective.
I added a few more comments and agree with all the points raised by @chrzaszcz

On a side note, it looks like some part of the changes could be excrated to separate PR. This would help with review.

src/amoc_config/amoc_config.hrl Show resolved Hide resolved
src/amoc_config/amoc_config_attributes.erl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config_attributes.erl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config.hrl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config_attributes.erl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config_env.erl Outdated Show resolved Hide resolved
src/amoc_scenario.erl Outdated Show resolved Hide resolved
src/amoc_scenario.erl Outdated Show resolved Hide resolved
test/amoc_config_attributes_SUITE.erl Outdated Show resolved Hide resolved
@DenysGonchar
Copy link
Collaborator Author

Also, I don't really get what kind of erlang module can be considered as ordinary looking at the code inside the get_module_type function.

those that are not configurable (parametrized) and not scenarios.

@DenysGonchar
Copy link
Collaborator Author

Yes, I don't think there is a strong need to support both formats. Amoc-arsenal uses an explicit amoc version so we can adapt the scenarios to new format when updating amoc in amoc-arsenal.

@michalwski Tide is also using an old format, so it must be updated as well. is it still ok to remove it?

@chrzaszcz
Copy link
Member

Thanks for making the changes. There are a few remaining things I asked about. Otherwise, I think it starts looking good to merge.

Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. We are getting there :) I had few extra comments on the code.

Also, I noticed there is no update to the documentation. It looks like to me these changes need to be reflected in the doc. I'm not saying to add the documentation change to this PR, rather immedietally after this one is merged.

@@ -0,0 +1,75 @@
-module(dummy_helper).

-required_variable(#{name=>dummy_var, description=>"dummy_var",
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespaces, please :) I'm not going to point every single place where there is a missing space. I hope this is not a big deal to take a look at the diff and pay attention to them. There is also the lint plugin for rebar3 which may be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cannot find any reference to lint plugin on rebar3.org I was thinking about elvis integration in the project to "fix" this once and forever :)

src/amoc_scenario.erl Outdated Show resolved Hide resolved
src/amoc_config/amoc_config_attributes.erl Show resolved Hide resolved
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.

All the changes I considered necessary are done. I think it is good enough to merge as soon as @michalwski approves it as well.

Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

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

It looks good now. Thanks.

@michalwski michalwski merged commit 7e426f3 into master Jul 1, 2020
@michalwski michalwski deleted the settings-rework branch July 1, 2020 19:28
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.

3 participants