You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Jonathan Hurley <jh...@hortonworks.com> on 2014/10/01 04:38:34 UTC
Re: Review Request 26194: Admin : ambari-server sync-ldap - support
--existing, --users & --groups options
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26194/#review55047
-----------------------------------------------------------
Ship it!
Overall just nits; code looks straighforward.
ambari-server/src/main/python/ambari-server.py
<https://reviews.apache.org/r/26194/#comment95387>
get_ldap_event_spec_names doesn't try/catch the parsing of the supplied files. A malformed or unreadable file could cause an exception. Should get_ldap_event_spec_names be wrapped in a try/catch?
ambari-server/src/main/python/ambari-server.py
<https://reviews.apache.org/r/26194/#comment95386>
Python's documentation says that you don't need to supply a separator, but it implies it uses spaces to determine the splits.
The comment mentions that it's a CSV; should we be explicitly using a comma here? This way we're not at the mercy of an interal splitting algorithm change in a future Python version.
- Jonathan Hurley
On Sept. 30, 2014, 5:31 p.m., Tom Beerbower wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26194/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2014, 5:31 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-7569
> https://issues.apache.org/jira/browse/AMBARI-7569
>
>
> Repository: ambari
>
>
> Description
> -------
>
> The command line 'ambari-server sync-ldap' should support the options as follows ...
>
> {quote}
> "ambari-server sync-ldap --existing" which would would first look at the ambari database and only clean-up those users and groups. This way, it doesn't pull any new ones, it a) removes users that are in ambari but out of ldap, b) remove groups that are in ambari but out of ldap and c) syncs the group membership of the groups that are in ambari
>
> "ambari-server sync-ldap --users users.txt --groups groups.txt" which would allow people to just sync a specific set of users and groups from ldap to ambari. The .txt files would be the user and group ids to sync into ambari. This is going to be important for people that just want to bring "a specific set" of users + groups into ambari from ldap (not the "all"). And then once this has happened, running with --existing option periodically helps keep these "in sync"
> {quote}
>
>
> Diffs
> -----
>
> ambari-server/sbin/ambari-server 027bf87
> ambari-server/src/main/python/ambari-server.py 9059319
> ambari-server/src/test/python/TestAmbariServer.py 7f769f1
>
> Diff: https://reviews.apache.org/r/26194/diff/
>
>
> Testing
> -------
>
> Results :
>
> Tests run: 2076, Failures: 0, Errors: 0, Skipped: 16
>
> ...
>
> [INFO] Executed tasks
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 33:19.993s
> [INFO] Finished at: Tue Sep 30 17:15:25 EDT 2014
> [INFO] Final Memory: 38M/293M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Tom Beerbower
>
>
Re: Review Request 26194: Admin : ambari-server sync-ldap - support
--existing, --users & --groups options
Posted by Jonathan Hurley <jh...@hortonworks.com>.
> On Sept. 30, 2014, 10:38 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/python/ambari-server.py, line 3174
> > <https://reviews.apache.org/r/26194/diff/1/?file=709783#file709783line3174>
> >
> > Python's documentation says that you don't need to supply a separator, but it implies it uses spaces to determine the splits.
> >
> > The comment mentions that it's a CSV; should we be explicitly using a comma here? This way we're not at the mercy of an interal splitting algorithm change in a future Python version.
>
> Tom Beerbower wrote:
> I eventually pass the comma separated list as the 'names' attribute value to the ldap_sync_event REST API. On the above line of code, I'm not trying to split on commas. That code should split on whitespace and then join the string back together. I'm just trying to strip the whitespace out of the CSV string to clean it up for the REST API.
Thanks for the explanation.
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26194/#review55047
-----------------------------------------------------------
On Oct. 1, 2014, 8:43 a.m., Tom Beerbower wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26194/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 8:43 a.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-7569
> https://issues.apache.org/jira/browse/AMBARI-7569
>
>
> Repository: ambari
>
>
> Description
> -------
>
> The command line 'ambari-server sync-ldap' should support the options as follows ...
>
> {quote}
> "ambari-server sync-ldap --existing" which would would first look at the ambari database and only clean-up those users and groups. This way, it doesn't pull any new ones, it a) removes users that are in ambari but out of ldap, b) remove groups that are in ambari but out of ldap and c) syncs the group membership of the groups that are in ambari
>
> "ambari-server sync-ldap --users users.txt --groups groups.txt" which would allow people to just sync a specific set of users and groups from ldap to ambari. The .txt files would be the user and group ids to sync into ambari. This is going to be important for people that just want to bring "a specific set" of users + groups into ambari from ldap (not the "all"). And then once this has happened, running with --existing option periodically helps keep these "in sync"
> {quote}
>
>
> Diffs
> -----
>
> ambari-server/sbin/ambari-server 027bf87
> ambari-server/src/main/python/ambari-server.py 9059319
> ambari-server/src/test/python/TestAmbariServer.py 7f769f1
>
> Diff: https://reviews.apache.org/r/26194/diff/
>
>
> Testing
> -------
>
> Results :
>
> Tests run: 2076, Failures: 0, Errors: 0, Skipped: 16
>
> ...
>
> [INFO] Executed tasks
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 33:19.993s
> [INFO] Finished at: Tue Sep 30 17:15:25 EDT 2014
> [INFO] Final Memory: 38M/293M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Tom Beerbower
>
>
Re: Review Request 26194: Admin : ambari-server sync-ldap - support
--existing, --users & --groups options
Posted by Tom Beerbower <tb...@hortonworks.com>.
> On Oct. 1, 2014, 2:38 a.m., Jonathan Hurley wrote:
> > Overall just nits; code looks straighforward.
Thanks for reviewing!
> On Oct. 1, 2014, 2:38 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/python/ambari-server.py, line 3089
> > <https://reviews.apache.org/r/26194/diff/1/?file=709783#file709783line3089>
> >
> > get_ldap_event_spec_names doesn't try/catch the parsing of the supplied files. A malformed or unreadable file could cause an exception. Should get_ldap_event_spec_names be wrapped in a try/catch?
Good point. I'll add it.
> On Oct. 1, 2014, 2:38 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/python/ambari-server.py, line 3174
> > <https://reviews.apache.org/r/26194/diff/1/?file=709783#file709783line3174>
> >
> > Python's documentation says that you don't need to supply a separator, but it implies it uses spaces to determine the splits.
> >
> > The comment mentions that it's a CSV; should we be explicitly using a comma here? This way we're not at the mercy of an interal splitting algorithm change in a future Python version.
I eventually pass the comma separated list as the 'names' attribute value to the ldap_sync_event REST API. On the above line of code, I'm not trying to split on commas. That code should split on whitespace and then join the string back together. I'm just trying to strip the whitespace out of the CSV string to clean it up for the REST API.
- Tom
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26194/#review55047
-----------------------------------------------------------
On Sept. 30, 2014, 9:31 p.m., Tom Beerbower wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26194/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2014, 9:31 p.m.)
>
>
> Review request for Ambari, Jonathan Hurley and Nate Cole.
>
>
> Bugs: AMBARI-7569
> https://issues.apache.org/jira/browse/AMBARI-7569
>
>
> Repository: ambari
>
>
> Description
> -------
>
> The command line 'ambari-server sync-ldap' should support the options as follows ...
>
> {quote}
> "ambari-server sync-ldap --existing" which would would first look at the ambari database and only clean-up those users and groups. This way, it doesn't pull any new ones, it a) removes users that are in ambari but out of ldap, b) remove groups that are in ambari but out of ldap and c) syncs the group membership of the groups that are in ambari
>
> "ambari-server sync-ldap --users users.txt --groups groups.txt" which would allow people to just sync a specific set of users and groups from ldap to ambari. The .txt files would be the user and group ids to sync into ambari. This is going to be important for people that just want to bring "a specific set" of users + groups into ambari from ldap (not the "all"). And then once this has happened, running with --existing option periodically helps keep these "in sync"
> {quote}
>
>
> Diffs
> -----
>
> ambari-server/sbin/ambari-server 027bf87
> ambari-server/src/main/python/ambari-server.py 9059319
> ambari-server/src/test/python/TestAmbariServer.py 7f769f1
>
> Diff: https://reviews.apache.org/r/26194/diff/
>
>
> Testing
> -------
>
> Results :
>
> Tests run: 2076, Failures: 0, Errors: 0, Skipped: 16
>
> ...
>
> [INFO] Executed tasks
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 33:19.993s
> [INFO] Finished at: Tue Sep 30 17:15:25 EDT 2014
> [INFO] Final Memory: 38M/293M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Tom Beerbower
>
>