Set generic environment values in JEXL context as `environment.*` instead of `normandy.*`

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
11 months ago
5 months ago

People

(Reporter: leplatrem, Assigned: glasserc)

Tracking

Trunk
Firefox 66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

11 months ago
Since Bug 1451033, some of the environment values are available in a generic ClientEnvironment class. Since Bug 1451031, Remote Settings uses this environment class as context in the JEXL filter expressions.

The current situation is:
- Normandy JEXL filter expressions can refer to both the generic environment values and Normandy specific values as ``normandy.*``
- RemoteSettings JEXL filter expressions can refer to both the generic environment values and remote settings specific values as ``environment.*``. 

In terms of developer experience and documentation (and probably UI), I believe it would be beneficial to have the generic environment values as ``environment.*`` and specific context values as ``normandy.*`` and ``remotesettings.*``. 

That way we would not have to duplicate documentation about generic values just because the context attribute is named differently.

Forward compatibility is easy, we could just set an alias so that old references (eg. ``normandy.version``) would continue to work in addition to the new ones (eg. ``environment.version``). I wonder how this would be feasible in terms of backward compat but I thought it was worth discussing.

Note: the remote settings specific values could almost be considered generic (app id and toolkit version). That's why I opened this bug in the Normandy client component.
I agree that we should unify on one namespace. For a while I was thinking of removing the prefix entirely (so `normandy.version` would become `version`). I had some technical difficulties getting this to work, due to the way mozjexl works. Putting all the values under a different, more generic, namespace seems like a good solution that would be easier to implement.

To bike shed a little, could we pick something shorter than `environment`? Maybe `context` or `env`?

As far as backwards compatibility, I suspect that we will have to put the new name and alias in place, and then eventually we could start using it, and then eventually remove all the old usages. It would be a long process. If we really wanted to expedite it, perhaps the Normandy server could automatically rewrite filter expressions.
Reporter

Comment 2

11 months ago
> I agree that we should unify on one namespace.

Cool! 

> To bike shed a little, could we pick something shorter than `environment`? Maybe `context` or `env`?

Let's go for ``env`` then :)

> If we really wanted to expedite it, perhaps the Normandy server could automatically rewrite filter expressions.

Yes, I agree. This ticket would consist in providing an alias in our clients. Until a sufficient amount of users runs the new code, we can rewrite the rules on the server side.
Assignee

Updated

6 months ago
Assignee: nobody → eglassercamp
Keywords: checkin-needed
We're doing a round of patch landings, and this patch has an accepted review and a blocking one. 
Is it all good for landing? Thank you.
Flags: needinfo?(eglassercamp)
Assignee

Comment 6

6 months ago
Yes, the pending review is from mythmon who is on PTO this week. Mat's review should be sufficiently powerful to land it.
Flags: needinfo?(eglassercamp)

Comment 7

6 months ago
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49241781bf81
offer context.environment as well as context.normandy r=leplatrem
Keywords: checkin-needed

Comment 8

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49241781bf81
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Assignee

Updated

5 months ago
Depends on: 1518292
You need to log in before you can comment on or make changes to this bug.