18F / micropurchase

18F's micro-purchase threshold experiment management app.
https://micropurchase.18f.gov
Other
68 stars 34 forks source link

Clean up credentials #1451

Closed baccigalupi closed 7 years ago

baccigalupi commented 7 years ago

Super huge applause for insisting on small single responsibility classes when it comes to understanding credentials.

When I was looking through those classes I noticed the same logic over and over again which is generally a sign of abstractions being broken at the wrong place.

It also seemed like there was a high overhead to adding and removing environmental variables.

So, this commit replaces the per envar type classes with three generalist classes that encompass the if statement that was seen over and over again:

  1. A class called Credentials that delegates to the other two classes depending on the Rails environment
  2. A class called Credentials::Local that looks for environmental variables on the top level with conventions for converting the cf namespace-y stuff into a full variable name.
  3. A class called Credentials::CloudFoundry that looks for envars where cups puts them.

In addition, I moved this small and not-growing set of classes into the model namespace.

harrisj commented 7 years ago

This looks great! Thank you. My only quibble is that there are a few Rubocop issues that need to be reformatted, but this is a nice conceptual refactoring that cleans things up

baccigalupi commented 7 years ago

Yeah, so those rubocop things. Don't agree with all of them, but having a standard is good. Will go ahead and clear it up

baccigalupi commented 7 years ago

@harrisj @adelevie Since this involves access to instance variables which are different on the server, feels like this should be thoroughly QA'd in a staging environment ... and I don't know the app well enough yet to do that. Can one of you volunteer. Happy to ride along on screenhero.

baccigalupi commented 7 years ago

The one big thing that is in Rubocop and could affect code function is the compact style of module and class vs the non-compact version. http://stackoverflow.com/a/24393133. It bit me and I had to be more explicit about the name of a class that should have been inside the namespace.

baccigalupi commented 7 years ago

Ok. Finally got passes!

harrisj commented 7 years ago

Slow Travis is slow. Sorry! I myself have disagreed with some of the Rubocop rules as well so I understand, but I guess it's for the Greater Good (Hot Fuzz reference implied)

Do you think we should formulate a checklist of things to investigate on staging before I merge this (which will deploy it to staging)? Otherwise, I would want to play with it quickly on staging once it is merged if that's cool with everybody

baccigalupi commented 7 years ago

The reality is that I don't know where those environmental variables are used. So, what I would really need from you all is a checklist of places that use environmental variables so that we can check staging for correct behavior.

harrisj commented 7 years ago

LGTM