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

Allow FileParser.Endpoints to be set via Ice::Properties #58

Merged
merged 2 commits into from
Apr 30, 2018

Conversation

mdorner
Copy link
Contributor

@mdorner mdorner commented Apr 27, 2018

If launched with --server as option, the icegridadmin command-line tool currently picks a random ephemeral port on localhost as endpoint. This PR enables it to read the endpoints from the property "FileParser.Endpoints" , and defaults to the old hard-coded endpoint, if it is not found.

I am forfeiting all rights to the code in alignment with CONTRIBUTING.md and the Contributor Agreement, and hope you can merge this trivial piece of code without requiring me to print and sign something.

@bernardnormier
Copy link
Member

A cleaner way to provide this functionality would be to:

  • use createObjectAdapter instead of createObjectAdapterWithEndpoints
  • use an IceGridAdmin.Xxx name for the object adapter, for example IceGridAdmin.Server
  • add this new OA name to the list of acceptable Ice-prefixed properties, see
    https://github.com/zeroc-ice/ice/blob/3.7/config/PropertyNames.xml#L469
    (you also need to regenerate and commit all the derived files with makeprops.py)
  • add code to give the default value to IceGridAdmin.Server.Endpoints

You could also submit a pull request for branch 3.7 instead of master, assuming you'd like to see this functionality in 3.7.x.

@bernardnormier bernardnormier self-requested a review April 27, 2018 19:43
@mdorner
Copy link
Contributor Author

mdorner commented Apr 28, 2018

Ok, I can adjust the PR to do that. The first three points seem straightforward to me, but I am not entirely sure how to default the value. Is there some place where these defaults typically go?

@bernardnormier
Copy link
Member

In general, you should set values for properties just before Communicator initialization because most Ice properties are read only during communicator initialization. Object adapter properties are read when the object adapter is created so here you could set the default value for IceGridAdmin.Server.Endpoints later.

Nevertheless, I suggest to set this default value before communicator initialization, i.e. change:
https://github.com/zeroc-ice/ice/blob/v3.7.1/cpp/src/IceGrid/Client.cpp#L244

_appName = args[0];
InitializationData id;
id.properties = createProperties(args);
id.properties->setProperty("Ice.Warn.Endpoints", "0"); // overwrite Ice.Warn.Endpoints
_communicator = initialize(id);

to something like:

_appName = args[0];
PropertiesPtr defaultProps = createProperties();
defaultProps->setProperty("IceGridAdmin.Server.Endpoints", "tcp -h localhost");
InitializationData initData;
initData.properties = createProperties(args, defaultProps);
initData.properties->setProperty("Ice.Warn.Endpoints", "0"); // overwrite Ice.Warn.Endpoints
_communicator = initialize(initData);

- Properly create IceGridAdmin.Server OA instead of using
createObjectAdapterwithEndpoints for FileParser
- Default IceGridAdmin.Server.Endpoints to old endpoints
- Add IceGridAdmin.Server to the list of acceptable properties
- Property files regenerated
@mdorner
Copy link
Contributor Author

mdorner commented Apr 28, 2018

I have made the changes as discussed, and hopefully as you would expect them.

As to where to merge them I am not sure what your branching-workflow is, but ideally I would like to see them going into both 3.6 and 3.7 eventually.

@bernardnormier bernardnormier changed the base branch from master to 3.7 April 30, 2018 14:29
@bernardnormier bernardnormier changed the base branch from 3.7 to master April 30, 2018 14:33
@bernardnormier bernardnormier merged commit 6809557 into zeroc-ice:master Apr 30, 2018
externl pushed a commit that referenced this pull request Apr 30, 2018
* Register FileParser on IceGridAdmin.Server adapter, used with --server option
@bernardnormier
Copy link
Member

Thanks for your contribution!

@mdorner
Copy link
Contributor Author

mdorner commented Apr 30, 2018

And thank you for your constructive feedback and help!

pepone pushed a commit that referenced this pull request Jan 16, 2019
* Register FileParser on IceGridAdmin.Server adapter, used with --server option
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.

2 participants