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

Expose parameterlist of current inform to the session context #272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

okraits
Copy link
Contributor

@okraits okraits commented Nov 9, 2017

This commit exposes the parameterlist of the current inform to the session context to enable the user to access it in a provision script via the path Events.Inform.ParameterList.

A valid usecase would be to check the parameterlist for any parameter of interest and then trigger needed actions to respond to the value change of that parameter.

@jameswmcnab
Copy link

This would be perfect for sending events to an external application (via a external script that uses HTTP or Redis pub-sub etc) when an inform occurs.

Do the provisions ever run at a time that is not triggered by an Inform?

@zaidka
Copy link
Member

zaidka commented Nov 30, 2017

I've been thinking about this after it was discussed in the mailing list a while back. This solution, while simple and straight forward, is not caching friendly. In a future release I plan to implement caching for provisions to skip executing a provision script if we know it won't have an effect (provision scripts are guaranteed to be idempotent). Not to go into details of how that works, the fact that you're representing all inform parameter names in a single parameter will cause unnecessary cache invalidation. If each inform parameter is to be represented separately then that would solve the caching problem.

@zaidka
Copy link
Member

zaidka commented Nov 30, 2017

Do the provisions ever run at a time that is not triggered by an Inform?

No. @jameswmcnab

@okraits
Copy link
Contributor Author

okraits commented Dec 18, 2017

Ok, so I want to propose the following modification:

  • add a NumberOfEntries parameter containing the number of inform parameters
  • present every inform parameter as a separate entry (possibly with the new value)

@okraits
Copy link
Contributor Author

okraits commented Jan 10, 2018

@zaidka Any opinion about this?

@okraits
Copy link
Contributor Author

okraits commented Mar 26, 2018

@zaidka I rewrote the patch. Now every inform parameter is added separately, with both path and value.

Please review.

@jameswmcnab
Copy link

jameswmcnab commented May 10, 2018

Any reason this can't be merged now the cache invalidation issue is resolved @zaidka?

@zaidka zaidka force-pushed the master branch 2 times, most recently from e8b7d94 to c836c73 Compare August 22, 2018 04:02
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