Skip to content
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

Python Flask example #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Python Flask example #1

wants to merge 5 commits into from

Conversation

albertoperdomo
Copy link
Member

No description provided.

python/.venv/bin/Activate.ps1 Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/requirements.txt Show resolved Hide resolved
python/app.py Show resolved Hide resolved
Copy link
Member

@WiNloSt WiNloSt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. The example also runs fine with a minor problem as commented inline.

app = Flask(__name__)

METABASE_SITE_URL = os.environ.get('METABASE_SITE_URL', 'http://localhost:3000')
METABASE_EMBEDDING_SECRET = os.environ.get('METABASE_EMBEDDING_SECRET')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a default value here. I ran the sample without reading the readme first. And it failed to run.

Suggested change
METABASE_EMBEDDING_SECRET = os.environ.get('METABASE_EMBEDDING_SECRET')
METABASE_EMBEDDING_SECRET = os.environ.get('METABASE_EMBEDDING_SECRET', 'ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff')

this key is used in most of our example keys, so it should be safe.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with that sample key? If not, it might be better to check and raise an error that the env key needs to be set before running, otherwise it will still be confusing. If we don't need to be fancy, changing get to os.environ['METABASE_EMBEDDING_SECRET'] would work as it will automatically raise a key error when not yet

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as @maxzheng says, adding a default value is just going to cover up the fact that the secret is missing. I prefer an explicit error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I'm on board with you guys. My gripe was that the error was quite cryptic. If os.environ['METABASE_EMBEDDING_SECRET'] throws better error that you need to set that key then let's do that.

python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
app = Flask(__name__)

METABASE_SITE_URL = os.environ.get('METABASE_SITE_URL', 'http://localhost:3000')
METABASE_EMBEDDING_SECRET = os.environ.get('METABASE_EMBEDDING_SECRET')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with that sample key? If not, it might be better to check and raise an error that the env key needs to be set before running, otherwise it will still be confusing. If we don't need to be fancy, changing get to os.environ['METABASE_EMBEDDING_SECRET'] would work as it will automatically raise a key error when not yet


METABASE_SITE_URL = os.environ.get('METABASE_SITE_URL', 'http://localhost:3000')
METABASE_EMBEDDING_SECRET = os.environ.get('METABASE_EMBEDDING_SECRET')
METABASE_EMBED_DASHBOARD_ID = int(os.environ.get('METABASE_EMBED_DASHBOARD_ID', '1'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for here as well. Default of 1 is ok if it should work, otherwise make it required instead like secret.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now going to be seeding instances with an example dashboard, which I expect to have ID 1, although I don't know if it's 100% guaranteed.

albertoperdomo and others added 3 commits April 17, 2024 17:38
Co-authored-by: Mahatthana (Kelvin) Nomsawadi <[email protected]>
Co-authored-by: Mahatthana (Kelvin) Nomsawadi <[email protected]>
Co-authored-by: Mahatthana (Kelvin) Nomsawadi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants