I would much prefer a backend with values fas2 or fas3 (or whatever the new project name is).
The fact that the new instance is IPA backed does not matter to the Sigul users.

Please make sure that pep8 or flake8 still pass over the codebase.
This seems to break that very much.

Any new features should get a test case added as well.
So please add that, and fix the comments I made and I can look further.

@puiterwijk thanks for the review!

wrt the tests, I'm looking through whats there already and I'll be honest I'm a little out of my depth. Would you be able to point me in the right direction? If never used autoconf before and the testing setup is very foreign to me.

I see some basic tests that l think I can add to but I'm unsure of how to set the config to use both fas and noggin and really how to actually test that that has worked then. Like I said, even some pointers on how to run the tests or set up the client so I can check expected output would help massively.

The new API is .get_user(username=username)

The new API is .get_group_members(groupname=group_name) (or will be very soon ;-) )

Actually I don't understand why you need to get the person from FASJSON, you seem to only check the group membership, and for that get_group_members() should give you all the info you need. You're not using the person variable afterwards.

This should be a fasjson_client.errors.ClientError (or should be very soon, I'm renaming the exceptions right now to be more usable)

I'm not sure you need both {0!s} and str(e), the !s should already cast it to string.

Thanks for the feedback. I've pushed the changes suggested. I am still unsure of tests, even if I could find where the current configuration is tested it would help. Actually, just some docs on how to run them would probably give me enough to figure it out.

Is it connecting to IPA to FAS?

Is it freeipa-fas, or noggin?

Why are you using str()? What is the type of member['username']?
If it's bytes, please use .decode('utf-8') instead.
If it's an str, why the conversion?

I'm still relatively sure this line is going to violate pep8.

@scoady To run the tests, it's a "standard" (autotools standard, Sigul is unfortunately not yet setuptools) autoreconf -vi && ./configure && make && make check.

Triple equal sign here. Someone has been writing javascript lately ;-)

Why does Sigul need to be aware of the user requesting things? That's all Koji-side, isn't it?

@ngompa note that sigul is also used in settings where there's no koji in use, so it has its own authentication as well. And in this case, the account system integration is used for authorization purposes: anyone who's not in a "signer" group gets kicked out immediately, without regard for which keys they have access to.

This has been sitting here for a while, is there anyone on this thread that could take a look at this?

I'm having trouble with the tests. I am unable to get any of the current tests to pass. If someone could let me know if they pass for them that would be great.

Otherwise, this needs to be progressed so that sigul can be tested on staging prior to FAS being turned off.

This has been sitting here for a while, is there anyone on this thread that could take a look at this?

I'm having trouble with the tests. I am unable to get any of the current tests to pass. If someone could let me know if they pass for them that would be great.

What kind of trouble are you having with the tests? Without knowing what type of trouble you have, there's not much I can do to help... You can ping me on IRC, and then I can try to help you either.

Otherwise, this needs to be progressed so that sigul can be tested on staging prior to FAS being turned off.

@puiterwijk at the moment i can't get them to pass on master, but I'm not familiar with autotools so maybe I'm doing something wrong.

I've pinged you on IRC but maybe you missed the messages. If you want to ping me whenever you are ready I hang around in fedora-aaa or fedora-apps. Thanks!

As I said on IRC a bit ago: there's now a runtests.sh script that will build and run a container for the tests.
That one does not test the keyctl binding method, but that should not be a problem.

@puiterwijk do you think this is in a mergable state now? The tests are now passing with my patch. Wrt the tests; I've had a look and I don't see any that test the current backend or flow.

@puiterwijk I'm back, sorry for the absence. Do you think you could take a look now and see if I'm on the right track? I've added a webserver that the test suite runs and then cleans up when it's done. I've made it mimic the is-member endpoint of fasjson so that list-users in sigul does that check using fasjson instead of FAS.