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

Duplicate experiment Key issue with multi feature flag #347

Merged
merged 8 commits into from
Jul 9, 2021

Conversation

The-inside-man
Copy link
Contributor

Summary

  • Updated changes to use experiment_id_map
  • Build experiment_key_map from experiment_id_map
  • Update decision service to id instead of experiment key
  • Add test cases

Test plan

  • FSC suite
    Issues

  • BugFix

@@ -41,6 +41,7 @@ def test_init(self):
self.config_dict['groups'][0]['trafficAllocation'],
)
}

Copy link

Choose a reason for hiding this comment

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

?

Copy link

Choose a reason for hiding this comment

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

Only curious because it was by itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need a new line to satisfy Flake8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatter...

@The-inside-man
Copy link
Contributor Author

Flake8 needs to be fixed... has both best practice and anti-pattern flagged so no winning:

./optimizely/project_config.py:577:17: W504 line break after binary operator
./optimizely/project_config.py:594:17: W503 line break before binary operator

@coveralls
Copy link

coveralls commented Jul 9, 2021

Coverage Status

Coverage decreased (-0.1%) to 95.646% when pulling 617cc4d on jbrown/bugfix-experimentKey into 8c3cfc1 on master.

Copy link
Contributor

@Mat001 Mat001 left a comment

Choose a reason for hiding this comment

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

LGTM. Good that FSC passes

@The-inside-man
Copy link
Contributor Author

@msohailhussain If you can give the final OK, I can then merge to Master.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm. please address comments before merge and add an FSC link passing this PR.

@@ -1,5 +1,5 @@
[flake8]
# E722 - do not use bare 'except'
ignore = E722
ignore = E722, W504
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please explain in the comments, why W504 added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment added explaining the reason for W504.

@@ -557,3 +566,35 @@ def is_feature_experiment(self, experiment_id):
"""

return experiment_id in self.experiment_feature_map

def get_variation_from_id_by_experiment_id(self, experiment_id, variation_id):
""" Gets experiment id and variation id
Copy link
Contributor

Choose a reason for hiding this comment

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

Gets only variation by variation id under specific experiment id.

Copy link
Contributor

Choose a reason for hiding this comment

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

please rephrase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comments changed to better describe what is happening.

return {}

def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key):
""" Gets experiment id and variation key
Copy link
Contributor

Choose a reason for hiding this comment

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

please rephrase and have more clear explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comments changed to better describe what is happening.

@The-inside-man
Copy link
Contributor Author

@The-inside-man The-inside-man merged commit ab89cf4 into master Jul 9, 2021
@The-inside-man The-inside-man deleted the jbrown/bugfix-experimentKey branch July 9, 2021 23:50
The-inside-man added a commit that referenced this pull request Jul 12, 2021
* Changing lookup of experiment in decision service to use experiment ID instead of key to resolve bug

* [BUGFIX] - Update to bucketer and added proper ID map to project_config.

* Switch to generate experiments_key_map from experiment_id_map rather than the other way around.

* Updated with unit tests.

* Change line break before operator to avoid W504 Flake8 issue.

* Flake8 changes to follow best practice and ignore the W504
The-inside-man added a commit that referenced this pull request Jul 15, 2021
* Changing lookup of experiment in decision service to use experiment ID instead of key to resolve bug

* [BUGFIX] - Update to bucketer and added proper ID map to project_config.

* Switch to generate experiments_key_map from experiment_id_map rather than the other way around.

* Updated with unit tests.

* Change line break before operator to avoid W504 Flake8 issue.

* Flake8 changes to follow best practice and ignore the W504
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

5 participants