Skip to content

Add non-dry, handle non-spec'd dbs, remove revoke all from permissions

Merge Request Checklist

Once your merge request is complete, please assign it a Meltano maintainer for review cycle. Once the review cycle finished, the reviewer shall approve the merge request so it can be merged by any Meltano maintainer.

Closes #1154 (closed)


This is (hopefully) a much more reasonable MR! This MR does 3 main things:

  • Add a non-dry execution mode to meltano permissions. Now when you leave off --dry it will actually execute the commands on Snowflake 😬
  • Sets revocations to only be from databases that are in the specification
  • Removes the --full-refresh functionality and code

Non-dry

I switched this up so that a status is passed along with each command. By default it's a None which will print as Skipped. If the command passes it's a Success and if it fails for any reason it's an Error. I tested with

    sql_grant_queries = [
         {"already_granted": True, "sql": "select 1"},
         {"already_granted": False, "sql": "grant usage on schema analytics.analytics to role engineer"},
         {"already_granted": False, "sql": "grant scpart on schema analytics.analytics to role engineer"}
     ]

The following screen shot shows all the permutations of --dry and --diff with the status. The commands were actually executed on the server!

Screen_Shot_2020-01-30_at_11.05.19_AM

Non-spec'd databases

I noticed previously that when the comparisons for the spec vs what was granted on the server came back that revocations were generated for databases not defined in the spec. We have a lot of non spec'd databases because of our clones. I ran the spec several times and nothing but raw, analytics, and snowflake dbs came back!

Remove full-refresh

After going through all of the revocations for dbs, schemas, and schema objects, I came to the conclusion that the full refresh command was really useless in its implementation. It was only looking at objects defined in the spec so it wasn't good for actually handling changes (i.e. if I remove your access in the spec from a database, and then ran full refresh, no revocations would happen for you on that database!)

I think we will want to add back a full-refresh command at some point, but it'll have to be smarter than the previous implementation.

Testing

I updated the necessary tests to get this MR to pass but did not add any additional ones. This MR completes the scope of work for a data team OKR and I will work in the coming months to continue to update the test suite!

Edited by Melty Bot

Merge request reports