-
Notifications
You must be signed in to change notification settings - Fork 3
feat: integrate superset feature #245
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
base: main
Are you sure you want to change the base?
Conversation
…superset_integration
…superset_integration
fe75d9b to
19b9ab7
Compare
19b9ab7 to
3622b1a
Compare
mrjones-plip
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really love the intention behind this PR! Making it easier to create superset users and to have a script to handle setting up encryption is powerful!
I'm just commenting on the manage-secrets.sh part of the PR. I did not test this or look at any of the type script. I'm using the conventional commits system for feedback.
I think the manage-secrets.sh has very good intentions, but I have concerns about shipping it to first time users. While I see a substantial amount of work was put in (and then I gave specific feedback on that work!) The reason is that sops already has a whole work flow that we're then wrapping a bash script around. I had quite a bit of difficulty getting the script work and it failed open (cleartext secrets on disk). While we might be able to fix these, I fear even more confusion might ensure by end users who don't appreciate the severity of their actions.
Instead, if we greatly simplify the process to document the sops usage steps in the readme, users will know they're encrypting files, leaving files in the clear occasionally, and will know how to debug anything if the secrets file or sops has any issues. I think documenting a happy path to create the secrets file should be pretty straight forward!
Finally - I realize we don't specify which env vars go into the secrets file. Or did I miss that and we do?
I'd like to hear what @Hareet has to say!
|
Woo @mrjones-plip, that was quick, thanks for looking into this, I am going to carefully check your feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome work. Something else to consider is the update flow; for roles and RLS. As new tables are added, the permissions need to be updated for both the template and all other roles created from it.
mrjones-plip
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing my feedback!
I gave the script another try and hit some snags - see comments below.
mrjones-plip
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had another thought on simplifying encryption process! See below
…perset_integration
3d66b7a to
78fb70c
Compare
78fb70c to
92a14fb
Compare
|
@paulpascal Sorry if I didn't follow up - do you need more review from me? Lemme know - happy to help |
alexosugo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Looking forward to seeing this in action. We should consider the update flows as well in later iterations; for (re)moved users and scope changes for roles and RLS.
13af92f to
b223401
Compare
#191