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

Warn when using secrets without --trusted #719

Conversation

helenlyang
Copy link
Contributor

@helenlyang helenlyang commented Nov 7, 2023

Summary

This updates the CLI to print a warning if a Truss has secrets defined in its config but is pushed without --trusted.

Testing

truss push

Commands below were for pushing a Truss with a dummy secret in config.yaml:

secrets:
  dummy_access_token: null

Without --trusted: poetry run truss push
image

With --trusted: poetry run truss push --trusted
Output:

Compressing... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
Uploading... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
✨ Model test-model was successfully pushed ✨

|---------------------------------------------------------------------------------------|
| Your model has been deployed as a draft. Draft models allow you to                    |
| iterate quickly during the deployment process.                                        |
|                                                                                       |
| When you are ready to publish your deployed model as a new version,                   |
| pass `--publish` to the `truss push` command. To monitor changes to your model and    |
| rapidly iterate, run the `truss watch` command.                                       |
|                                                                                       |
|---------------------------------------------------------------------------------------|

🪵  View logs for your deployment at https://app.baseten.co/models/7wl16p0q/versions/qrjrg13/logs

Copy link

linear bot commented Nov 7, 2023

BT-8704 Add warning when using secrets without trusted

The trusted flag in truss push is not super discoverable. If you have a Truss with secrets in it, we should:

  • Add a warning if you haven't used the --trusted flag
  • Update the missing secrets exceptions in Truss to verify with the user that they used the --trusted flag

Some additional context

What is the --trusted flag?

When deploying a model with Baseten, there's often a need to have secrets. Check out the baseten guide on how to use Secrets: https://docs.baseten.co/observability/secrets

https://truss.baseten.co/guides/secrets

Code Pointers

The change to make here is in the Truss CLI (See Repo: https://github.com/basetenlabs/truss). This file is the entrypoint to the CLI: https://github.com/basetenlabs/truss/blob/main/truss/cli/cli.py

how to print a warning: https://github.com/basetenlabs/truss/pull/717/files

The easiest path to developing in this repo is to use Github Codespaces.

truss/cli/cli.py Outdated
Comment on lines 419 to 423
not_trusted_text = """
Warning: your Truss has secrets but was not pushed with --trusted.
Please push with --trusted to grant access to secrets.
"""
click.echo(not_trusted_text)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feedback on messaging, formatting, etc. welcome!

I could also stack this PR on top of #717 and use console.print(not_trusted_text, style=yellow).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just merged #717, so you can merge master and use the colors

One other thought here, how does this look if we put it after the "Model was successfully pushed" copy? I worry that it gets lost if it's above the big block of development model text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I think both changes help!

@helenlyang helenlyang requested a review from squidarth November 7, 2023 01:08
@helenlyang helenlyang marked this pull request as ready for review November 7, 2023 05:20
Copy link
Collaborator

@squidarth squidarth left a comment

Choose a reason for hiding this comment

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

lgtm, awesome work @helenlyang! one small note, from the screenshot, the yellow might be a little subtle, it might be better as red.

another non-blocking this is that it might be good to pull the check into a config_warnings function that we could add more warnings too, but I think it's ok to run without that

@helenlyang
Copy link
Contributor Author

Thanks @squidarth! Made the readability changes and updated the screenshot. I added a TODO for the config_warnings item--think it might make sense to add that once we have multiple config checks

@squidarth
Copy link
Collaborator

TODO for the config_warnings item--think it might make sense to add that once we have multiple config checks

agreed

@helenlyang helenlyang merged commit 5340a38 into main Nov 7, 2023
3 checks passed
@helenlyang helenlyang deleted the helenyang/bt-8704-add-warning-when-using-secrets-without-trusted branch November 7, 2023 21:26
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.

2 participants