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

add --no-admin flag to registration script #3836

Merged
merged 9 commits into from Sep 28, 2018

Conversation

Projects
None yet
4 participants
@bwindels
Contributor

bwindels commented Sep 10, 2018

As the title says, I would like this option to be able to register non-admin users without interaction for the riot-web end-to-end tests, and thought it was a good way for a first synapse PR :)

@bwindels bwindels requested a review from erikjohnston Sep 10, 2018

@erikjohnston

Woo, thanks, just a few nits

help="Register new user as an admin. Will prompt if --regular-user is not set.",
)
parser.add_argument(
"--regular-user",

This comment has been minimized.

@erikjohnston

erikjohnston Sep 11, 2018

Member

Traditionally this would probably be called --no-admin, to make it clearer that its the opposite of --admin.

register_new_user(args.user, args.password, args.server_url, secret, args.admin)
if args.admin and args.regular_user:
print "Both --admin and --regular-user are set, choose either."
sys.exit(1)

This comment has been minimized.

@erikjohnston

erikjohnston Sep 11, 2018

Member

Rather than doing this manually, we should probably use the inbuilt argparse conflict functionality: https://docs.python.org/2/howto/argparse.html#conflicting-options

print "Both --admin and --regular-user are set, choose either."
sys.exit(1)
admin = True if args.admin else False if args.regular_user else None

This comment has been minimized.

@erikjohnston

erikjohnston Sep 11, 2018

Member

I honestly can't remember how multiple if statements like this work, can we write split it out into actual if statements please?

(You can also write this as args.admin or args.regular_user or None, but that's a bit special too)

@richvdh

This comment has been minimized.

Member

richvdh commented Sep 18, 2018

Fixes #2310

@spantaleev

This comment has been minimized.

Contributor

spantaleev commented Sep 18, 2018

Instead of having 2 different flags (--admin and --regular-user) and trying to detect conflicts, why not just:

  • drop the current --admin argument
  • introduce a new --type=admin|user argument, which defaults to 'user'.. Or defaults to None and asks interactively

It's backward-incompatible, but at least it does things cleanly.

@bwindels

This comment has been minimized.

Contributor

bwindels commented Sep 19, 2018

Instead of having 2 different flags (--admin and --regular-user) and trying to detect conflicts, why not just:

* drop the current `--admin` argument

* introduce a new `--type=admin|user` argument, which defaults to `'user'`.. Or defaults to `None` and asks interactively

It's backward-incompatible, but at least it does things cleanly.

Opted for --no-admin because it's backwards compatible as you mentioned.

@bwindels

This comment has been minimized.

Contributor

bwindels commented Sep 19, 2018

Not sure why the tests are failing though ...

@richvdh

This comment has been minimized.

Member

richvdh commented Sep 19, 2018

Not sure why the tests are failing though ...

try merging latest develop; if nothing else, it will make circleci tell us why the tests are failing.

@spantaleev

This comment has been minimized.

Contributor

spantaleev commented Sep 19, 2018

As it is now:

  • case 1: if only --admin is passed, admin = True. This is ok. ✔️

  • case 2: if only --no-admin is passed, args.admin would be False and args.no_admin would be False (?!), which makes admin = False or False or None, which yields admin = None. This is wrong.. And the conditional is also very weird.

  • case 3: if neither --admin, nor --no-admin is passed, args.admin would be False and args.no_admin would be True (?!), which makes admin = True. This is wrong.

So it seems like it's generally wrong now and also backward-incompatible (see case 3).

@bwindels

This comment has been minimized.

Contributor

bwindels commented Sep 19, 2018

Travis says:

Looking at these files:
----
1. /home/travis/build/matrix-org/synapse/scripts/register_new_matrix_user
----
No new newsfragments found on this branch.
ERROR: InvocationError for command '/home/travis/build/matrix-org/synapse/.tox/check-newsfragment/bin/python -m towncrier.check --compare-with=origin/develop' (exited with code 1)
___________________________________ summary ____________________________________
ERROR:   check-newsfragment: commands failed
The command "tox -e $TOX_ENV" exited with 1.
@richvdh

This comment has been minimized.

Member

richvdh commented Sep 20, 2018

No new newsfragments found on this branch.

In other words: "Please add a changelog fragment as per CONTRIBUTING.rst"

@bwindels

This comment has been minimized.

Contributor

bwindels commented Sep 27, 2018

Not sure why the tests are failing, but trying if merging in develop helps ...

@bwindels bwindels changed the title from add --regular-user flag to registration script to add --no-admin flag to registration script Sep 27, 2018

@bwindels

This comment has been minimized.

Contributor

bwindels commented Sep 28, 2018

Tests on Travis seem to have passed, but they're still appearing as pending here for some reason ...

@richvdh richvdh self-requested a review Sep 28, 2018

@richvdh

lgtm

@richvdh

This comment has been minimized.

Member

richvdh commented Sep 28, 2018

Tests on Travis seem to have passed, but they're still appearing as pending here for some reason ...

this happens occasionally if there is a problem when Travis comes to send the memo to github that the builds completed. I've restarted them.

@richvdh

This comment has been minimized.

Member

richvdh commented Sep 28, 2018

(thanks for bearing with us on this, @bwindels!)

@richvdh richvdh merged commit 9a8bbc9 into develop Sep 28, 2018

6 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment