-
Notifications
You must be signed in to change notification settings - Fork 183
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
Feat: Add environments command to list environments #3770
base: main
Are you sure you want to change the base?
Conversation
sqlmesh/cli/main.py
Outdated
@cli_analytics | ||
def environments(obj: Context, expiry_ds: bool) -> None: | ||
"""Prints the list of SQLMesh environments with its expiration datetime.""" | ||
environments = {e.name: e.expiration_ts for e in obj.state_sync.get_environments()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads up that environment objects are pretty large and loading and parsing them all at once can be a pretty long and memory-intensive operation for big projects with a lot of environments.
A better approach could be having a specialized method in the the state_sync.py
that only fetches limited fields like names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I'll do this. Thanks :)
sqlmesh/cli/main.py
Outdated
environment_names = list(environments.keys()) | ||
if expiry_ds: | ||
max_width = len(max(environment_names, key=len)) | ||
print( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be using the console
instance here as well.
) | ||
@line_magic | ||
@pass_sqlmesh_context | ||
def environments(self, context: Context, line: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the exact copy-paste of the code above. It might be worse considering having a method as part of Context
that prints this.
Thanks a lot for your contribution! Left some comments |
76e3f18
to
eda4fa7
Compare
639bdd6
to
91d134b
Compare
@izeigerman, have reworked the PR based on your comments, really appreciate if you re-review. Thanks :) |
Fixes #3732
I came across this feature request and found it interesting. I made some assumptions and created this PR. Please review and share your feedback. thanks :)