Feat: Add environments command to list environments #3770
Conversation
|
Thanks a lot for your contribution! Left some comments |
lafirm
force-pushed
the
list_environments
branch
10 times, most recently
from
76e3f18 to
eda4fa7
Compare
lafirm
force-pushed
the
list_environments
branch
from
639bdd6 to
91d134b
Compare
|
@izeigerman, have reworked the PR based on your comments, really appreciate if you re-review. Thanks :) |
| @abc.abstractmethod | ||
| def get_environment_names( | ||
| self, get_expiry_ts: bool = True | ||
| ) -> t.Optional[t.List[t.Tuple[str, ...]]]: | ||
| """Fetches all environment names along with expiry datetime if get_expiry_ts is True. | ||
| Returns: | ||
| A list of all environment names along with expiry datetime if get_expiry_ts is True. | ||
| """ |
There was a problem hiding this comment.
- This will make the PR breaking, FYI.
- I wonder if we want to make this more generic, i.e., being able to query any state table, possibly selecting only a certain subset of fields. I don't feel strongly about this, but the current API feels somewhat weird, because the method is "get names" but we're including a flag to fetch another field. I'll defer to Iaroslav.
There was a problem hiding this comment.
yeah, I understand your point but,
I wonder if we want to make this more generic, i.e., being able to query any state table, possibly selecting only a certain subset of fields
- can be more generic, but I haven't used any new method in this PR but tweaked
_environments_querymethod to select only the fields we want. And I feel that making a generic method should be a separate PR, however I really appreciate your view on this. @izeigerman, @georgesittas
I don't feel strongly about this, but the current API feels somewhat weird, because the method is "get names" but we're including a flag to fetch another field.
- I've seen this kind of method in many places, where we add an arg and that makes the method name partially true. One for ex. is plan method in our project which not only plans but apply the changes when we add arg
auto_apply. I felt that its okay, but happy to change if you guys disagree.
| def environments(self, context: Context, line: str) -> None: | ||
| """Prints the list of SQLMesh environments with its expiry datetime.""" | ||
| args = parse_argstring(self.environments, line) | ||
| context.print_environment_names(show_expiry=args.show_expiry) |
There was a problem hiding this comment.
[Nit] this is an opinionated styling suggestion, don't necessarily have to do it in this PR if others agree with it:
Active SQLMesh environments (<number>):
├── prod
├── dev_1
...
└── dev_n
There was a problem hiding this comment.
"Active" in this messaging implies that we'd only be showing unexpired environments. Haven't cross-checked if this is true in this PR. Perhaps something worth considering?
There was a problem hiding this comment.
I think that the actual use case is to find the list of unexpired (active) envs and invalidate it and run the janitor to cleanup. What you're saying makes sense but its good to see the expired envs (to run janitor) as well I believe, can't think of any use case around showing all the environments including the expired envs except when the janitor has been skipped for a long time.
While thinking about this, I got a random question - what'd happen in the backend when I've an expired environment called env1 and I created a plan for the same env1. will it rejuvenate the env1 or invalidate, janitor process and create the env1
lafirm
force-pushed
the
list_environments
branch
from
95c3ac7 to
18e1e8c
Compare
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 :)