GAM
GAM copied to clipboard
GAM-team
Argparse
Have you thought about using the argparse library to parse user input rather than just iterating over sys.argv? I find it difficult to memorize GAM commands, which lack built-in documentation, don't distinguish between flags and input, and are inconsistent with other command line tools I'm familiar with. Obviously this would be a major change to the codebase, but I think it would substantially improve usability as well as make the code much easier to maintain.
Let me know what you think! Alex
I was looking at it a bit.
I think one of the issues I see with implementing argparse in a nice way is actually structure of the commands.
They're not grouped by item we're taking action on, but by action itself, so it's:
gam update
Not sure what's gam policy on changes breaking backwards compatibility.
I have some proof of concept now:
pbm@tauri:~/GAM/src$ ./gam.py group
usage: gam.py group [-h] {info,delete} ...
gam.py group: error: too few arguments
pbm@tauri:~/GAM/src$ ./gam.py group info
usage: gam.py group info [-h] [--nousers] [--noaliases] [--groups] name
gam.py group info: error: too few arguments
Looks like moving to argparse would require quite a big changes in gam CLI interface. If I'm reading argparse docs correctly all flags would have to have "--" prefix. Also to make it nicer from code point of view and (I think) also from the user point of view, we could group functionality as I mentioned before - basically by swapping
gam info group -> gam group info
gam delete group -> gam group delete, etc
Also in few places I noticed inconsistent naming - for example there is gam calendar showacl to show ACLs, but function to add ACLs is just gam calendar add. That could be cleaned up as well.
@jay0lee: what's your opinion on that?
I'm open to doing this but it's going to take some work. Rough thoughts:
- I'm somewhat embarrassed by argument parsing in GAM today. sys.argv[i] all over everywhere is just ugly :-(
- Pretty sure that in order to use argparse or something else, we'd need to break compatability with older GAM commands. I'm not opposed to using more standard -- style arguments though it will be a process for admins to pickup and learn the new commands.
- Using argparse or similar should give us auto-generated help docs in some form or another which would be a huge win in terms of maintainng the wiki and answering command format questions on the discussion group.
My suggestion is to start small here. Start clean with a very small set of GAM APIs and commands, maybe something like users and groups. We can focus on a clean, simple and scalable format for these commands and then once its' agreed upon at that point, take on the work of moving the boatload of other APIs / commands into the new format.
Pawel,
Send me a private email.
Ross
On Wed, Mar 13, 2019 at 10:16 AM Paweł Szubert [email protected] wrote:
I have some proof of concept now:
pbm@tauri:~/GAM/src$ ./gam.py group usage: gam.py group [-h] {info,delete} ... gam.py group: error: too few arguments
pbm@tauri:~/GAM/src$ ./gam.py group info usage: gam.py group info [-h] [--nousers] [--noaliases] [--groups] name gam.py group info: error: too few arguments
Looks like moving to argparse would require quite a big changes in gam CLI interface. If I'm reading argparse docs correctly all flags would have to have "--" prefix. Also to make it nicer from code point of view and (I think) also from the user point of view, we could group functionality as I mentioned before - basically by swapping and : gam info group -> gam group info gam delete group -> gam group delete, etc
Also in few places I noticed inconsistent naming - for example there is gam calendar showacl to show ACLs, but function to add ACLs is just gam calendar add. That could be cleaned up as well.
@jay0lee https://github.com/jay0lee: what's your opinion on that?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jay0lee/GAM/issues/677#issuecomment-472520152, or mute the thread https://github.com/notifications/unsubscribe-auth/AIU8L8HnDvG_3svsMOTcDsyArP0hSNzZks5vWTJdgaJpZM4RlEwN .
-- Ross Scroggs [email protected]
Pawel,
I'm not sure why you would want to rewrite Jays' gam, I've already done that and you have a huge amount of work in front of you just to recode the parsing. You're left with a version lacking a significant number of features. Python 2: https://github.com/taers232c/GAMADV-XTD Python 3: https://github.com/taers232c/GAMADV-XTD3
Here's an example: Basic Gam:
gam info group <GroupItem> [nousers] [noaliases] [groups]
def doGetGroupInfo(group_name=None):
cd = buildGAPIObject(u'directory')
gs = buildGAPIObject(u'groupssettings')
getAliases = getUsers = True
getGroups = False
if group_name is None:
group_name = normalizeEmailAddressOrUID(sys.argv[3])
i = 4
else:
i = 3
while i < len(sys.argv):
myarg = sys.argv[i].lower()
if myarg == u'nousers':
getUsers = False
i += 1
elif myarg == u'noaliases':
getAliases = False
i += 1
elif myarg == u'groups':
getGroups = True
i += 1
elif myarg in [u'nogroups', u'nolicenses', u'nolicences', u'noschemas', u'schemas', u'userview']:
i += 1
if myarg == u'schemas':
i += 1
else:
systemErrorExit(2, '%s is not a valid argument for "gam info group"' % myarg)
Advanced GAM:
gam info group|groups <GroupEntity> [members] [managers] [owners] [nousers|notsuspended|suspended] [quick]
[noaliases] [groups] <GroupFieldName>* [fields <GroupFieldNameList>] [formatjson]
def doInfoGroups():
infoGroups(getEntityList(Cmd.OB_GROUP_ENTITY))
def infoGroups(entityList):
cd = buildGAPIObject(API.DIRECTORY)
getAliases = getUsers = True
getGroups = getSettings = False
FJQC = FormatJSONQuoteChar()
groups = []
members = []
cdfieldsList = gsfieldsList = isSuspended = None
entityType = Ent.MEMBER
rolesSet = set()
while Cmd.ArgumentsRemaining():
myarg = getArgument()
if myarg == u'quick':
getAliases = getUsers = False
elif myarg == u'nousers':
getUsers = False
elif myarg in SUSPENDED_ARGUMENTS:
isSuspended = _getIsSuspended(myarg)
entityType = [Ent.MEMBER_NOT_SUSPENDED, Ent.MEMBER_SUSPENDED][isSuspended]
elif myarg == u'noaliases':
getAliases = False
elif myarg in GROUP_ROLES_MAP:
rolesSet.add(GROUP_ROLES_MAP[myarg])
getUsers = True
elif myarg == u'groups':
getGroups = True
elif myarg in GROUP_FIELDS_CHOICE_MAP:
if not cdfieldsList:
cdfieldsList = [u'email',]
addFieldToFieldsList(myarg, GROUP_FIELDS_CHOICE_MAP, cdfieldsList)
if myarg in [u'name', u'description']:
if not gsfieldsList:
gsfieldsList = []
gsfieldsList.append(myarg)
elif myarg in GROUP_ATTRIBUTES:
if not gsfieldsList:
gsfieldsList = []
gsfieldsList.extend([GROUP_ATTRIBUTES[myarg][0]])
elif myarg == u'collaborative':
if not gsfieldsList:
gsfieldsList = []
gsfieldsList.extend(COLLABORATIVE_ACL_ATTRIBUTES)
elif myarg == u'fields':
if not cdfieldsList:
cdfieldsList = [u'email',]
if not gsfieldsList:
gsfieldsList = []
for field in _getFieldsList():
if field in GROUP_FIELDS_CHOICE_MAP:
addFieldToFieldsList(field, GROUP_FIELDS_CHOICE_MAP, cdfieldsList)
if field in [u'name', u'description']:
gsfieldsList.append(field)
elif field in GROUP_ATTRIBUTES:
gsfieldsList.extend([GROUP_ATTRIBUTES[field][0]])
elif field == u'collaborative':
gsfieldsList.extend(COLLABORATIVE_ACL_ATTRIBUTES)
else:
invalidChoiceExit(list(GROUP_FIELDS_CHOICE_MAP)+list(GROUP_ATTRIBUTES), True)
# Ignore info user arguments that may have come from whatis
elif myarg in INFO_USER_OPTIONS:
if myarg == u'schemas':
getString(Cmd.OB_SCHEMA_NAME_LIST)
else:
FJQC.getFormatJSON(myarg)
sys.argv[i] all over everywhere is just ugly :-( All of the sys.argv processing is handled in functions
-- Ross Scroggs [email protected]
Gam has a lot of inconsistency (gam info user ... vs gam user verb ...) and if the commandline syntax were reworked that'd be awesome, but it would need to be an optional method to prevent backward compatibility issues. For example 'gamc' as the program name to let it know the new syntax is in use, or use argument syntax instead of positional syntax, ala 'gam --user bri --info', so it could key off of the fact that there are double dash arguments to know it's the new syntax.
Anything else would cause chaos.
The implementation trick is that since everything currently is positionally based, the functions for processing the remaining sys.argv options are now embedded within a function. You'd essentially need to move all the actual API-calling code to their own functions, and have two entry points to them.
Doable, yes. Doable quickly, no. Which is why I never went down that road.
After I wrote this issue I was motivated to implement it in a wrapper to Google APIs: https://github.com/Cloudbakers/agm
You all may find it interesting / useful or draw inspiration from it
Again, it was a very early decision on my part with GAM to not use something like argparse so that GAM could have what I felt would be a more "natural" human readable language for commands. That decision has created a lot of inconsistency challenges over the years though.
I feel like if we're going to do this we should just go ahead and do it right and rely fully on something like argparse. No doubt it'll be a huge challenge for admins to move over to a rewritten GAM that uses argparse but the consistency and self-documenting capabilities will also make self-discovery a lot easier process for everyone.
Also being open source and fairly self contained offers a decent workaround to "backward compatible". If you want backward compatability, use the old version :-) If you need new APIs and features you'll need to move to and learn the new version syntax.
As an example, how would argparse handle this:
<UserAttributes> ::=
(address clear|(type work|home|other|(custom <String>) [unstructured|formatted <String>] [pobox <String>] [extendedaddress <String>] [streetaddress <String>]
[locality <String>] [region <String>] [postalcode <String>] [country <String>] [countrycode <String>] notprimary|primary))|
(agreed2terms|agreedtoterms <Boolean>)|
(changepassword|changepasswordatnextlogin <Boolean>)|
(crypt|sha|sha1|sha-1|md5|nohash)|
(customerid <String>)|
(email|primaryemail|username <EmailAddress>)|
(otheremail clear|(work|home|other|<String> <String>))|
(externalid clear|(account|customer|login_id|network|organization|<String> <String>))|
(firstname|givenname <String>)|
(gal|includeinglobaladdresslist <Boolean>)|
(gender clear|(female|male|unknown|(other <String>) [addressmeas <String>]))|
(im clear|(type work|home|other|(custom <String>) protocol aim|gtalk|icq|jabber|msn|net_meeting|qq|skype|yahoo|(custom_protocol <String>) <String> [notprimary|primary]))|
(ipwhitelisted <Boolean>)|
(keyword clear|(occupation|outlook|(custom <string>) <String>))|
(language clear|<LanguageList>)|
(lastname|familyname <String>)|
(location clear|(type default|desk|<String> area <String> [building|buildingid <BuildingID>] [floor|floorname <FloorName>] [section|floorsection <String>] [desk|deskcode <String>] endlocation))|
(note clear|([text_plain|text_html] <String>|(file <FileName> [charset <Charset>])))|
(organization clear|([type domain_only|school|unknown|work] [customtype <String>] [name <String>] [title <String>] [department <String>] [symbol <String>]
[costcenter <String>] [location <String>] [description <String>] [domain <String>] notprimary|primary))|
(org|ou|orgunitpath <OrgUnitPath>)
(password random|<Password>)|
(phone clear|([type work|home|other|work_fax|home_fax|other_fax|main|company_main|assistant|mobile|work_mobile|pager|work_pager|car|radio|callback|isdn|telex|tty_tdd|grand_central|(custom <String>)]
[value <String>] notprimary|primary))|
(posix clear|(username <String> uid <Integer> gid <Integer> [system|systemid <String>] [home|homedirectory <String>] [shell <String>] [gecos <String>] [primary <Boolean>] endposix))|
(relation clear|(spouse|child|mother|father|parent|brother|sister|friend|relative|domestic_partner|manager|dotted-line_manager|assistant|admin_assistant|exec_assistant|referred_by|partner|<String> <String>))|
(sshkeys clear|(key <String> [expires <Integer>] endssh))|
(suspended <Boolean>)|
(website clear|(home_page|blog|profile|work|home|other|ftp|reservations|app_install_page|<String> <URL> [notprimary|primary]))|
(<SchemaName>.<FieldName> [multivalued|multivalue|value|multinonempty [type work|home|other|(custom <String>)]] <String>)
gam update user <UserItem> <UserAttributes>* [clearschema <SchemaName>] [clearschema <SchemaName>.<FieldName>]
Most objects would likely map to a argparse subcommand: https://docs.python.org/3/library/argparse.html#sub-commands
So something like: gam user --action create --email [email protected] --firstname Jay --lastname Lee --password S3cr3t
Another option would be to take a hint from Google Cloud's gcloud command which is pretty modular and use calliope: https://github.com/google-cloud-sdk/google-cloud-sdk https://github.com/edlane/cliapi
There are two inter-mingled issues here:
- How to parse the arguments - Changing this will be long and hard
- A cleaner syntax - Not so difficult
Currently many non-user oriented commands look like this:
gam verb noun <Instance> <Options>
For example:
gam create group <EmailAddress> <GroupAttributes>*
gam update group <GroupItem> [admincreated <Boolean>] [email <EmailAddress>] <GroupAttributes>*
gam update group <GroupItem> add [owner|manager|member] [notsuspended|suspended] [allmail|daily|digest|none|nomail] <UserTypeEntity>
gam update group <GroupItem> delete|remove [owner|manager|member] <UserTypeEntity>
gam update group <GroupItem> sync [owner|manager|member] [notsuspended|suspended] [allmail|daily|digest|none|nomail] <UserTypeEntity>
gam update group <GroupItem> update [owner|manager|member] [notsuspended|suspended] [allmail|daily|digest|none|nomail] <UserTypeEntity>
gam update group <GroupItem> clear [member] [manager] [owner] [notsuspended|suspended]
gam delete group <GroupItem>
gam info group <GroupItem> [nousers] [noaliases] [groups]
Allowing this form as well would be simple and would present no backwards compatibility issues:
gam noun verb <Instance> <Options>
For example:
gam group create <EmailAddress> <GroupAttributes>*
gam group update <GroupItem> [admincreated <Boolean>] [email <EmailAddress>] <GroupAttributes>*
gam group update <GroupItem> add [owner|manager|member] [notsuspended|suspended] [allmail|daily|digest|none|nomail] <UserTypeEntity>
gam group update <GroupItem> delete|remove [owner|manager|member] <UserTypeEntity>
gam group update <GroupItem> sync [owner|manager|member] [notsuspended|suspended] [allmail|daily|digest|none|nomail] <UserTypeEntity>
gam group update <GroupItem> update [owner|manager|member] [notsuspended|suspended] [allmail|daily|digest|none|nomail] <UserTypeEntity>
gam group update <GroupItem> clear [member] [manager] [owner] [notsuspended|suspended]
gam group delete <GroupItem>
gam group info <GroupItem> [nousers] [noaliases] [groups]
The coding changes to do this are minimal.
I would focus first on cleaning up the syntax , Advanced GAM has done a lot of this already:
New syntax:
gam calendar <CalendarEntity> create|add acls <CalendarACLRole> <CalendarACLScopeEntity> [sendnotifications <Boolean>]
gam calendar <CalendarEntity> update acls <CalendarACLRole> <CalendarACLScopeEntity> [sendnotifications <Boolean>]
gam calendar <CalendarEntity> delete acls [<CalendarACLRole>] <CalendarACLScopeEntity>
gam calendar <CalendarEntity> info acls <CalendarACLScopeEntity> [formatjson]
gam calendar <CalendarEntity> show acls [formatjson]
gam calendar <CalendarEntity> print acls [todrive <ToDriveAttribute>*] [formatjson] [quotechar <Character>]
gam calendar <CalendarEntity> create|add event [id <String>] <EventCreateAttribute>+
gam calendar <CalendarEntity> import event icaluid <iCalUID> <EventImportAttribute>+
gam calendar <CalendarEntity> update events <EventEntity> <EventUpdateAttribute>+
gam calendar <CalendarEntity> delete events <EventEntity> [doit] [notifyattendees]
gam calendar <CalendarEntity> purge events <EventEntity> [doit] [notifyattendees]
gam calendar <CalendarEntity> wipe events
gam calendar <CalendarEntity> move events <EventEntity> to <CalendarItem> [notifyattendees]
gam calendar <CalendarEntity> empty calendartrash
Old syntax:
gam calendar <CalendarEntity> create|add <CalendarACLRole> ([user] <EmailAddress>)|(group <EmailAddress>)|(domain [<DomainName>])|default [sendnotifications <Boolean>]
gam calendar <CalendarEntity> update <CalendarACLRole> ([user] <EmailAddress>)|(group <EmailAddress>)|(domain [<DomainName>])|default [sendnotifications <Boolean>]
gam calendar <CalendarEntity> delete [<CalendarACLRole>] ([user] <EmailAddress>)|(group <EmailAddress>)|(domain [<DomainName>])|default
gam calendar <CalendarEntity> showacl [formatjson]
gam calendar <CalendarEntity> addevent <EventCreateAttribute>+
gam calendar <CalendarEntity> deleteevent <EventEntity> [doit] [notifyattendees]
gam calendar <CalendarEntity> wipe
The old syntax is supported for backwards compatibility.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
![stale[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
