[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Pulp-list] Fwd: CDS: gofer authentication

Thanks for you comments, Jason.

On 03/18/2011 11:40 AM, Jay Dobies wrote:
Hash: SHA1

One more thing:

             secret = self._cds_stub(cds).initialize()
             return secret
         except RequestTimeout, e:
             raise CdsTimeoutException(e), None, sys.exc_info()[2]
         except DispatchError, e:
             raise CdsCommunicationsException(e), None, sys.exc_info()[2]
         except NotAuthorized, e:
             raise CdsAuthException(e), None, sys.exc_info()[2]
         except Exception, e:
             raise CdsMethodException(e), None, sys.exc_info()[2]

NotAuthorized is a subclass of DispatchError, so that code block will
never get executed. Need to reverse the order on those.

Excellent catch.

On 03/18/2011 12:38 PM, Jay Dobies wrote:

I pushed the pulp ->  external CDS authentication feature.

Commit: cf449c22299e01e872dff9b92a3cff65ac39cc36

Mind doing a quick review?  I'll be making a pass though the unit tests


Looks pretty solid. Most of my comments focus around things you'll run
into in one way or another when you do the unit tests, though you may
have approaches in mind and I'm just not thinking of.

- init_cds():  Method now returns the secret, don't forget to update the
docstring to mention that and say if it's a string or something more

All of the method docstrings are missing parameter specifications.  I'll add them too.

- There aren't any tests for this (sorry about that, my bad) so you'll
have to add the test file itself.

- How are you going to override the location of the secret file for unit
tests? The path can be specified but the object is constructed inside of
getsecret(). The default is to read from config, but that's loaded by
gofer (there may be some way of overriding config values loaded by
gofer, which is one reason I'm asking since I don't know).

Well, not sure how to unit test this anyway. The plugin needs to run with gofer so it's not really a unit test. Seems like it needs to be tested in system tests? Does it make sense to require the CDS plugin be configured in gofer and require qpidd/goferd running for unit tests to pass? Let's discuss.

- We'll need tests that run both with the secret file present (scenario
of normal usage) and not present (scenario of an initialize). When
cleaning up the test runs by deleting the secret file directory, won't
we run into an issue since Secret is a singleton, is already loaded, so
__mkdir won't get called again?

See [1] below.

- The caching doesn't come into play if gofer is restarted. The secret
is cached on write, but not on read. So if the CDS is bounced after it's
initialized, the caching isn't used.

Crap, thought I updated the cache in read().  good catch.  Again, see [1] below.

- In my experience, singletons have always been a headache in unit tests
since modules aren't unloaded between runs, so you never know exactly
which test is the one causing the instantiation. Is there a benefit to
this approach over simply having module methods read_secret,
write_secret, and delete_secret and caching in the module itself?

Well, since the modules are only loaded once, implementing as functions and module variables presents the same headaches for testing. I view the "secret" as an persistent object with read/write/delete operations. Making it a singleton was really an after thought for performance.

Making it an object was a matter of philosophy (nope, not going to open up this debate here). Is there a benefit beyond those well-known about OO programming? No. If there needs to be, then 95% of our classes in pulp should not have been classes. That said, I'd be glad to rewrite as functions and module variables (it would take about 10 min).

[1] Actually, the more I think about it, making it a singleton and caching the secret really wasn't warranted. Plus, it would be nice to be able to reset the secret without bouncing gofer.

Pulp-list mailing list
Pulp-list redhat com

- --
Jay Dobies
RHCE# 805008743336126
Freenode: jdob
Version: GnuPG v2.0.14 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/


Pulp-list mailing list
Pulp-list redhat com

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]