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

Update starburst-galaxy-quickstart.yaml #196

Merged
merged 3 commits into from Jun 22, 2022

Conversation

starburst-blumbert
Copy link

@starburst-blumbert starburst-blumbert commented Mar 9, 2022

Add additional leading "*" on line #66 to bold the "TRINO_PASSWORD" text.

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • For commits that came from upstream, [UPSTREAM] has been prepended to the commit message
  • JIRA link(s):
  • The Jira story is acked
  • An entry has been added to the latest build document in Build Announcements Folder.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious)
  • The developer has manually tested the changes and verified that the changes work.

Add additional leading "*" on line red-hat-data-services#66 to bold the "TRINO_PASSWORD" text.
Copy link

@taneem-ibrahim taneem-ibrahim left a comment

Choose a reason for hiding this comment

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

/lgtm

@taneem-ibrahim
Copy link

@bdattoma Would you like to review these changes? I have approved the PR.

@lugi0
Copy link

lugi0 commented May 5, 2022

I think this PR already takes care of it, but I am unsure of how the dashboard handles formatting so I just wanted to point out that in the current version the underscores are missing from the variable names, and they are instead formatted in italics, like TRINOUSERNAME
Since the example notebook expects the env variables to be set with the underscore, could you double check that is show up correctly? Like TRINO_USERNAME?

@bdattoma
Copy link

bdattoma commented May 5, 2022

@taneem-ibrahim I've tried these changes on local and it confirmed what @lugi0 said. The underscore needs to be escaped like this **TRINO\_USERNAME**

The starburst sample notebook expects to receive TRINO_USERNAME: https://github.com/starburstdata/rhods-starburst-galaxy-101/blob/main/explore-data.ipynb

@starburst-blumbert
Copy link
Author

@bdattoma @lugi0 Unfortunately, I do not yet have a way to preview this content locally. Do you have any documentation you can provide me on how to do so?

@bdattoma
Copy link

bdattoma commented May 5, 2022

@bdattoma @lugi0 Unfortunately, I do not yet have a way to preview this content locally. Do you have any documentation you can provide me on how to do so?

@starburst-blumbert you could try in this way:

  1. git clone the dashboard repo (your fork in this case)
  2. move inside the folder
  3. checkout the branch where the change you need to check is
  4. run "npm install" from your terminal
  5. run "npm run dev" from your terminal
  6. wait some seconds, and the dashboard will automatically open in your browser

Made "JupyterHub" bold on line red-hat-data-services#63 for consistency with other instructions.

Added escape characters ("\") on line 66 for the Trino environment variables so that they display properly.

Removed the additional "your" that was on line 84.

Shortened line 99 to remove the comment about Python code and running models in Jupyter.
@starburst-blumbert
Copy link
Author

Thank you for the tips @bdattoma ! I have made the updates to the environment variable names as suggested along with a couple of other minor cosmetic issues I found when previewing this asset. Please let me know if there is anything else that needs correcting.

Added in a new line (red-hat-data-services#84) to provide instructions for downloading the Jupyter notebook.

Updated step numbers for lines 85 - 88 accordingly since a new step 1 was added.
@Jooho
Copy link

Jooho commented Jun 22, 2022

/lgtm

@Jooho Jooho merged commit db171a0 into red-hat-data-services:master Jun 22, 2022
Copy link

@bdattoma bdattoma left a comment

Choose a reason for hiding this comment

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

lgtm

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.

None yet

5 participants