Add additional checks in pantheon:fetch-db for expired tokens #970
Conversation
Summary of ChangesHello @mtift, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the reliability and user experience of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the pantheon:fetch-db task by adding checks for required variables and enhancing error handling for terminus commands. These changes make the task more robust and provide a better user experience. I have provided a couple of suggestions to further improve the error handling by allowing terminus's own error messages to be displayed, which could offer more specific diagnostic information to the user when something goes wrong. Overall, this is a valuable enhancement.
|
|
||
| # Authenticate with failure check, silencing the original Terminus output | ||
| - echo "⚡ Authorising with Pantheon..." | ||
| - terminus auth:login --machine-token="${PANTHEON_TOKEN}" >/dev/null 2>&1 || { echo "❌ Authentication failed. Your PANTHEON_TOKEN value is likely incorrect or expired. Please verify it in your .env file."; exit 1; } |
There was a problem hiding this comment.
By redirecting both stdout and stderr to /dev/null, you might be hiding valuable error information from terminus. If authentication fails, terminus could print a specific reason to stderr (e.g., 'invalid token', 'expired token'). It would be beneficial to let this message be displayed to the user for easier debugging. Consider only redirecting stdout to keep the success output clean, but allow stderr to pass through in case of an error.
- terminus auth:login --machine-token="${PANTHEON_TOKEN}" >/dev/null || { echo "❌ Authentication failed. Your PANTHEON_TOKEN value is likely incorrect or expired. Please verify it in your .env file."; exit 1; }There was a problem hiding this comment.
I am curious what the normal output is.
|
|
||
| # Fetch database with failure check, silencing only the error output to preserve the progress bar | ||
| - echo "⚡ Fetching database from Pantheon..." | ||
| - terminus backup:get {{.PANTHEON_SITE_ID}}.$ENVIRONMENT --element=db --to=$DB_DIR/db.sql.gz 2>/dev/null || { echo "❌ Database fetch failed. Please check your PANTHEON_SITE_ID and ensure the token has permission for this site."; exit 1; } |
There was a problem hiding this comment.
Similar to the authentication step, redirecting stderr to /dev/null prevents the user from seeing specific error messages from terminus if the backup fetch fails. The custom error message is helpful, but the original error could provide more context (e.g., 'backup not found', 'environment does not exist'). I recommend removing the stderr redirection to improve diagnostics. While the comment mentions preserving the progress bar, this is often handled on stdout or terminus may be smart about TTY detection.
- terminus backup:get {{.PANTHEON_SITE_ID}}.$ENVIRONMENT --element=db --to=$DB_DIR/db.sql.gz || { echo "❌ Database fetch failed. Please check your PANTHEON_SITE_ID and ensure the token has permission for this site."; exit 1; }
deviantintegral
left a comment
deviantintegral
left a comment
There was a problem hiding this comment.
Should we move authentication to a separate task we call first? That way, we don't have to duplicate the authentication workflow in multiple tasks.
| - if [ -z "$PANTHEON_TOKEN" ]; then echo "❌ PANTHEON_TOKEN is not set. Please add it to your .env file."; exit 1; fi | ||
| - if [ -z "{{.PANTHEON_SITE_ID}}" ]; then echo "❌ PANTHEON_SITE_ID is not set. Please add it to your .env file."; exit 1; fi |
There was a problem hiding this comment.
Since pantheon tokens are not scoped (ie one I generate for myself can access every site I have access to in the dashboard), I think it's our recommendation to put them in the global ddev settings?
| - if [ -z "{{.PANTHEON_SITE_ID}}" ]; then echo "❌ PANTHEON_SITE_ID is not set. Please add it to your .env file."; exit 1; fi | ||
|
|
||
| # Authenticate with failure check, silencing the original Terminus output | ||
| - echo "⚡ Authorising with Pantheon..." |
There was a problem hiding this comment.
| - echo "⚡ Authorising with Pantheon..." | |
| - echo "⚡ Authorizing with Pantheon..." |
|
|
||
| # Authenticate with failure check, silencing the original Terminus output | ||
| - echo "⚡ Authorising with Pantheon..." | ||
| - terminus auth:login --machine-token="${PANTHEON_TOKEN}" >/dev/null 2>&1 || { echo "❌ Authentication failed. Your PANTHEON_TOKEN value is likely incorrect or expired. Please verify it in your .env file."; exit 1; } |
There was a problem hiding this comment.
I am curious what the normal output is.
Closes #969