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

Add export filter by inputtag #86

Closed

Conversation

Paul-Blanchaert
Copy link
Contributor

@Paul-Blanchaert Paul-Blanchaert commented Nov 24, 2021

The goal of the change is to export smui rules by inputtag.

This change basically allows to export the SMUI rules in different querqy rewriter files based on an input tag value. This "flexible" assignment of smui rules to different querqy rewriters can be useful for e.g. A/B testing of SMUI rules.

The branch "add_export_filter_by_inputtag" in the o19s fork contains the code to export different files for a predefined (via env var) inputtag (e.g. inputtag “tenant”). When that configuration is set, the export behavior would change: instead of generating 1 rewriter txt file, it would create 1 rewriter txt file per “tenant” code (e.g. “common_rules_A” with rules with inputtag “tenant” = “A”, “common_rules_B” with rules with inputtag “tenant” = “B”, ...)

An additional change by dobestler is to allow to export based on the inputtag that is provided in the REST request rather than via env var. That would be a cleaner and more flexible solution. A new endpoint like GET /api/v1/allRulesTxtFilesByTag/{tag} instead of the environment var would be nice... but should probably be extended with tagname and tagvalue: GET /api/v1/allRulesTxtFilesByTag/{tagname}/{tagvalue} and cater for a way to export all tagvalues at once via GET /api/v1/allRulesTxtFilesByTag/{tagname} when tagvalue is absent.

@dobestler
Copy link
Contributor

Hi @Paul-Blanchaert
Does your PR enable the export of rules having set certain inputtag(s) via REST interface? Sorry, I was too lazy to properly look at your changes. I am asking because I wanted to extend the REST endpoint GET /api/v1/allRulesTxtFiles to take a list of inputtags to only export the rules with these inputtags.

BR
Dominic

@Paul-Blanchaert
Copy link
Contributor Author

Paul-Blanchaert commented Nov 26, 2021 via email

@Paul-Blanchaert
Copy link
Contributor Author

Hi @dobestler,

As for your question re the REST endpoint GET /api/v1/allRulesTxtFiles: the current version of the pull request will result in a zip file with all files being split per value of the configured inputtag property (e.g. for inputtag property = tenant, this would be: common_rules.txt (=rules without inputtag tennant), common_rules_AA.txt (=rules with inputtag tennant=AA), common_rules_BB.txt (=rules with inputtag tennant=BB), ...)).
So it should be a matter of mapping the req parms from /api/v1/allRulesTxtFiles into the inputtag property (that is currently on config parameter) and the values of the inputtag (currently all defined values of the configured inputtag property) into the current logic.

@dobestler
Copy link
Contributor

What is left for this PR to be merged? I am interested in this functionality

@Paul-Blanchaert
Copy link
Contributor Author

I believe we should try to convince @mkr. mkr seems to be the current committer for smui... This will also need some rebase-ing as well.

@pbartusch
Copy link
Collaborator

mkr seems to be the current committer for smui...

... ?

@Paul-Blanchaert
Copy link
Contributor Author

When I look at the commits on querqy/smui in 2022, they are all commited by mkr it seems

@pbartusch
Copy link
Collaborator

@Paul-Blanchaert
Copy link
Contributor Author

Top!
Sorry for my wrong assumption!
Can you give your input on the open question of dobestler: What is left for this PR to be merged?

@dobestler
Copy link
Contributor

dobestler commented Feb 6, 2023

I would like to move this forward and I can offer help as well. @Paul-Blanchaert Maybe you could rebase to come a step closer?

Feature question: Do you think it might be a bit clearer if the input tag is part of a new endpoint like GET /api/v1/allRulesTxtFilesByTag/{tag} instead of the environment var?

@pbartusch What's your opinion?

@pbartusch
Copy link
Collaborator

I am lacking context for the use case and the SMUI version that is running (OSC branch vs. SMUI-main), but I am fine with additional API features under the same version (v1) as long as they are backwards compatible.

@mkr
Copy link
Collaborator

mkr commented Feb 7, 2023

Thank you, I'm happy to push this through but can we please first

  • describe the feature and the reasoning behind it in a few words
  • rebase the code against master
  • do our best and adhere to Scala idioms like not using null when trying to parameterize function behaviour

@Paul-Blanchaert
Copy link
Contributor Author

Paul-Blanchaert commented Feb 7, 2023

The goal of the change is to export smui rules by inputtag.

This change basically allows to export the SMUI rules in different querqy rewriter files based on an input tag value. This "flexible" assignment of smui rules to different querqy rewriters can be useful for e.g. A/B testing of SMUI rules.

The branch "add_export_filter_by_inputtag" in the o19s fork contains the code to export different files for a predefined (via env var) inputtag (e.g. inputtag “tenant”). When that configuration is set, the export behavior would change: instead of generating 1 rewriter txt file, it would create 1 rewriter txt file per “tenant” code (e.g. “common_rules_A” with rules with inputtag “tenant” = “A”, “common_rules_B” with rules with inputtag “tenant” = “B”, ...)

An additional change by dobestler is to allow to export based on the inputtag that is provided in the REST request rather than via env var. That would be a cleaner and more flexible solution. A new endpoint like GET /api/v1/allRulesTxtFilesByTag/{tag} instead of the environment var would be nice... but should probably be extended with tagname and tagvalue: GET /api/v1/allRulesTxtFilesByTag/{tagname}/{tagvalue} and cater for a way to export all tagvalues at once via GET /api/v1/allRulesTxtFilesByTag/{tagname} when tagvalue is absent.

Unfortunately, I don't have the time to work on SMUI, so I can't help to get this in a cleaner state now... When it would be helpful, I can always send a patch with the changes of the branch.
The changes are quite limited, so it shouldn't be too hard to get this in a good shape and dobestler also offered to help out.

@pbartusch
Copy link
Collaborator

I will close this PR under the following assumption:

Goal of this export feature was to realize draft rules. For this exists another approach today ( #124 , #127 (comment) ) and another way forward with future versions ( #127 ).
All known multi tenant requirements can be dealt with existing SMUI features (pls add to this, if you are aware of requirements that are not currently handleable).

Feel free to re-open or add to this comment in case you have a different perspective or knowledge about other use cases that this feature would support.

@pbartusch pbartusch closed this Mar 3, 2023
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.

4 participants