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

Stream constructor uses the DEFLECT_ID and DEFLECT_HOST ENV_VARs #98

Merged
merged 1 commit into from
Jun 6, 2016

Conversation

rdumusc
Copy link

@rdumusc rdumusc commented Jun 2, 2016

No description provided.

* @param host The address of the target Server instance. It can be a
* hostname like "localhost" or an IP in string format like
* "192.168.1.83". If set, the environment variable DEFLECT_HOST
* takes precedence over the given value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the precedence. Let's say a UI allows the user to enter host+port to connect to - know the app has now way of ensuring that this user-provided values are used. Imo the typical use case is that env variables provide defaults, overridable by command line args, overridable by GUI (cf. $DISPLAY, '--display' usage).

Copy link
Author

Choose a reason for hiding this comment

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

That's a fair point. @tribal-tec argued the other way around, that ENV variables should be able to override runtime behaviour without recompiling. Maybe command line arguments should still be able to take precedence?

Copy link
Contributor

@eile eile Jun 3, 2016

Choose a reason for hiding this comment

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

Imo the default (env, argv) construction should have a different ctor. zeroeq::http::Server has the create() factory method which may return a nullptr - arguably not the most elegant way. The alternative proposed by @tribal-tec was to have the ctor throw if no entity is desired, but this requires the callee to catch - also not great.

@rdumusc rdumusc force-pushed the master branch 2 times, most recently from ab46d69 to 0e2eb25 Compare June 3, 2016 12:58
@rdumusc
Copy link
Author

rdumusc commented Jun 3, 2016

Updated with precedence for the programmatic arguments

@tribal-tec
Copy link
Contributor

+1 for me

@rdumusc rdumusc merged commit 1cb94bc into BlueBrain:master Jun 6, 2016
tribal-tec added a commit to tribal-tec/Equalizer that referenced this pull request Jun 6, 2016
tribal-tec added a commit to tribal-tec/Equalizer that referenced this pull request Jun 6, 2016
tribal-tec added a commit to tribal-tec/Equalizer that referenced this pull request Jun 7, 2016
eile added a commit to Eyescale/Equalizer that referenced this pull request Jun 7, 2016
Change Deflect streaming activation, according to BlueBrain/Deflect#98
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