-
Notifications
You must be signed in to change notification settings - Fork 2
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
Allow arbitrary python code #530
Conversation
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.
Sorry ^^
I still think some docs would be great (with the usual experimental disclaimer)
Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
…stackabletech/superset-operator into feature/abitrary-python-imports
Excellent point, done with 4b64920 |
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.
Some tests would be good. Especially testing if role group and role configOverrides work as expected.
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.
LGTM % Razvans 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.
approved as discussed
The decision is not public, can you summarize what this is about and if it is something that is documented please include a link to the docs. If it's not documented can you briefly summarize why note? Does this need to be mentioned in the release notes? |
This is to allow the user to add arbitrary python code to the We've got it documented in our superset docs and airflow docs For a customer we added a whole class and reference to it to this config file s.th. they were able to use a custom We've reached a decision on that this is how superset wants to be configured ( no matter how ugly it might appear ) and thus we want the operator to be useful in this case. We've had an additional customer for airflow who wrote their whole config for themself ( basically ignoring the operators work and thus it's not used ) to achieve having own classes and functions. They now can use the configs the operator provides and have their custom code as a config. I can't really make a objective statement weather or not we should mention it in the release notes. For me it should as it enables some customers and they might wanna know. Left to do: We have a draft PR in docker images where we can include necessary libraries to use OIDC with superset properly ( airflow might wanna have that too, to be discussed ). |
Thank you. In that case: Can you please add a short sentence as it could appear in the release notes as a comment here? |
Sure Release Notes: |
Description
This PR is the outcome of the decision https://github.com/stackabletech/decisions/issues/21
Contains:
..Default::default()
from lib.rs as we don't want default values ( can be forgotten )writeln!
Definition of Done Checklist
Author
Reviewer
Acceptance