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

Add note to end of deployment #983

Closed
wants to merge 4 commits into from
Closed

Add note to end of deployment #983

wants to merge 4 commits into from

Conversation

iameskild
Copy link
Member

Fixes | Closes | Resolves #

Please remove anything marked as optional that you don't need to fill in. Choose one of the keywords preceding to refer to the issue this PR solves, followed by the issue number (e.g Fixes # 666). If there is no issue, remove the line. Remove this note after reading.

Changes:

Types of changes

What types of changes does your code introduce?

Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe): small note to improve the user-experience during deployment

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests?

  • Yes
  • No

Further comments (optional)

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered and more.

@iameskild iameskild requested a review from danlester January 3, 2022 00:53
@danlester
Copy link
Contributor

Thanks for this. Only thing is that at the moment keycloak and keycloak.initial_root_password are optional in the schema.

Maybe they shouldn't be, although an admin might like to remove initial_root_password from the YAML file after initial deployment, especially if they used a password they used elsewhere etc.

So anyway, the point is that it is possible your log message will throw an error if those fields are omitted.

Having said that, I wouldn't want to bet that everything functions correctly if you omit the fields and do qhub deploy anyway... It uses 'password' as a default where initial_root_password is not provided.

Copy link
Contributor

@danlester danlester left a comment

Choose a reason for hiding this comment

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

I commented on the main conversation thread.

@iameskild
Copy link
Member Author

Thanks for the info @danlester! The main reason I wanted to add this is due to the confusion around where the admin should go to change the root user password. I have added a basic check to ensure that this doesn't error out.

(where is the main conversion thread?)

@danlester
Copy link
Contributor

Oh, I just added my original comment on the main PR, and then realised that maybe I was supposed to make a review, so I then just made a review with "I commented on the main conversation thread" in it. But it turns out GitHub just adds that to the same place anyway. So, basically just ignore my small second comment above!

Anyway, I still think the code doesn't quite take into account the schema. It is possible to have security.keycloak but not security.keycloak.initial_root_password in the YAML file.

I think you just need to do something like:

initial_password = config["security"].get("keycloak", {}).get("initial_root_password", "password")

And then just use initial_password to make up your log comment, which doesn't particularly need an if statement any more.

@costrouc
Copy link
Member

Closing for now since the PR #1003 added a message at the end of a deployment around these lines. We can improve upon it in another PR. Bellow is a sample.

[terraform]: INFO:qhub.provider.terraform:terraform apply took 7.778 [s]
INFO:qhub.provider.terraform:terraform=/tmp/terraform/1.0.5/terraform output directory=stages/08-enterprise-qhub
INFO:qhub.provider.terraform:terraform output took 1.465 [s]
QHub deployed successfully
Services:
 - conda_store -> https://quansight-beta.qhub.dev/conda-store/
 - dask_gateway -> https://quansight-beta.qhub.dev/gateway/
 - jupyterhub -> https://quansight-beta.qhub.dev/
 - keycloak -> https://quansight-beta.qhub.dev/auth/
 - monitoring -> https://quansight-beta.qhub.dev/monitoring/
Kubenetes kubeconfig located at file:///tmp/QHUB_KUBECONFIG
Kubecloak master realm username=root password=file:///tmp/QHUB_DEFAULT_PASSWORD
Additional administration docs can be found at https://docs.qhub.dev/en/stable/source/admin_guide/
INFO:qhub.deploy:deploying QHub took 161.609 [s]

@costrouc costrouc closed this Feb 13, 2022
@iameskild iameskild deleted the 20220102eae branch May 11, 2022 00:33
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