Closed Bug 1477255 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Normandy Client, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: leplatrem, Assigned: glasserc)

References

Details

Attachments

(1 file)

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.
> 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: 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)
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)
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
https://hg.mozilla.org/mozilla-central/rev/49241781bf81
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Depends on: 1518292
You need to log in before you can comment on or make changes to this bug.