deployer: create separate new typer apps under the deployer and update the general structure of the directory #3094
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
GeorgianaElena
force-pushed
the
deployer-generator
branch
6 times, most recently
from
f658af5 to
4f70111
Compare
GeorgianaElena
changed the title
There was a problem hiding this comment.
Thanks @GeorgianaElena! Generally looks good to me, made some minor suggestions.
There was a problem hiding this comment.
I love that tests is being renamed to involve health_checks, i consider that to fix #971!
GeorgianaElena
force-pushed
the
deployer-generator
branch
from
61b880e to
902ea9c
Compare
This was part of @yuvipanda's feedback <3 🚀
I've udpated based on your feedback, Yuvi and rebased & fix the conflicts. Thank you both and sorry for a hard to review PR 👀 |
deployer/infra_components/cluster.py
Outdated
| from .file_acquisition import get_decrypted_file, get_decrypted_files | ||
| from deployer.utils.env_vars_management import unset_env_vars | ||
| from deployer.utils.file_acquisition import ( | ||
| HELM_CHARTS_DIR, | ||
| get_decrypted_file, | ||
| get_decrypted_files, | ||
| ) | ||
| from deployer.utils.rendering import print_colour | ||
|
|
||
| from .hub import Hub |
There was a problem hiding this comment.
I expected from .hub to become from deployer.infra_components.hub given the other changes so that we consistently use either relative or absolute references.
I'm generally a bit uncertain about the choice between relative . references and absolute package_name references, not knowing the consequences of going for one, the other, or a mix.
There was a problem hiding this comment.
@consideRatio, the main motivation for switching the majority of imports from relative to absolute, is because with this PR at least 2 or 3 more levels are added and it relative imports were not serving their purpose anymore when you have something like ....file_acquisition import find_absolute_path_to_cluster_file
(Also, apparently there's a PEP that suggests using absolute ones https://peps.python.org/pep-0008/#imports)
But I believe relative imports are still useful for the deployer package when referring to the current directory.
For example, in deployer/commands/generate/dedicated_cluster/aws.py we have a very simple line that says:
from .dedicated_cluster_app import dedicated_cluster_app
if we turn it into an absolute import then it becomes:
from deployer.commands.generate.dedicated_cluster.dedicated_cluster_app import dedicated_cluster_app`
Although not recommended (relative imports are not recommended anyhow), I would leave these imports relative to the current directory be, for simplicity. @consideRatio, do you feel strongly about using absolute imports everywhere? If yes, I will update them.
I added a commit that uses absolute imports in a few more places, but haven't touched the ones in the commands directory for the reason above.
There was a problem hiding this comment.
do you feel strongly about using absolute imports everywhere? If yes, I will update them.
No, I'm mostly cautious because I recall issues that I didn't fully understand related to using one or the other kind.
leave these imports relative to the current directory be
Sure!
There was a problem hiding this comment.
The UI is far better with the sub-commands for sure, nice!
I expect this to be safe to merge for our production hubs operation since we have staging hubs also. I've not reviewed all commits or all changed lines, but skimmed through a bit.
To resolve:
- another merge conflict
- comment about
from .hubimport
|
@GeorgianaElena btw I saw a commit that was including |
Create a new typer app called `generate` that is registered under the main `deployer`. Under this new generate app, create two more other sub-apps called `dedicated-cluster` and `billing`. Then register the fuctions related to creating a new cluster under the `dedicated-cluster` app and the one that creates a new cost table, under the `billing` app. Group the files related to creating a new cluster under deployer/generate/dedicated_cluster and the ones related to billing under deployer/generate/billing.
Then register the functions that describe the cilogon management commands under this new app. Also, move the cilogon-client code in its own directory inside the deployer main dir.
Create a new exec sub-app under the main `deployer` app and create two more apps under it called `shell` and `debug`. - Move the old deployer/debug.py code under deployer/exec/debug/app_and_commands.py and register some of the functions there under the `deployer exec debug` command. - Move the deployer/cloud_access.py under deployer/exec/shell/cloud_commands.py and register the functions there under the `deployer exec shell` command. - Move the two functions related to shell execution inside the hub and homes dirs from the debug.py file into deployer/exec/shell/infra_components_commands.py
And move all of the code related to grafana management in a new location, under deployer/grafanai, including the `deploy_grafana_dashboads` from `deployer.py`
Also - rename some files to account for the fact that they contain functions registered as commands - add help strings to the new typer apps
Create a deployer/commands directory and put all of the code that defines deployer commands under this directory, including the main `deployer.py` file.
The nesting is now deep enough to make relative imports hard to follow,
so use their full path for clarity.
- Moves the validate function out of the deployer.py file into its own file - This file, toghether with the schema are now groupped under deployer/commands/validate - Allow to call individual validate commands for type of validation supported and have the original `deployer validate` be equivalent now with `deployer validate all`
- Move the `unset_env_vars` function out of utils and into cluster.py, the only placed it was used - Now, the functions in utils are all related to rendering, so we can call the file `rendering.py` and be less generic - Update all of the inports to match the new name
- From helm_upgrade_decission.py to file_acquisition.py - Because it is not only used by helm upgrade cmd, it makes more sense to be in this utility file instead.
- Group together the file_aquisition.py and rendering.py files into a utils directory because they are used throught the entire codebase. - Also update all the imports to match their new location
Rename some of the files so that they all follow some form of convention: - files that contain functions registered as typer app commends (have @<app>.command) need to end in `_cmd` or `commands` for clarity - files that define a new typer app need to have `app` somewhere in their name - deployer commands that don't require defining any new helper files (like cilogon) don't need to be into their own directory by themselves. Instead keep them in the parent `deployer/commands`, under a name follwing the two rules above
Update the way we call the deployer sub-commands in GitHub workflows to match the new structure.
Define two most used file locations (the root of the repo and the location of helm charts) inside the file_aquisition.py file and import and use them in all the places there were initially redifined. This way, we will need to update them only once and not everytime a file that uses them changes location.
and fix a few imports and add a missing help string
- present the general structure of the deployer directory - describe what is sub-directory and file is about - describe some of the file naming std that was followed
Update the README structure to match the structure of the directory: - there are standalone commands, directly registered under the `deployer`, like the ones in `commands/deployer.py` - there are sub-commands, registered under the `deployer` command, each of them with functions registered as sub-commands, and some even one more level dee The READMe now expresses the same structure as the package's
GeorgianaElena
force-pushed
the
deployer-generator
branch
from
7d4bae3 to
0e9e626
Compare
There was a problem hiding this comment.
Thank you for working this @GeorgianaElena!! Happy to see sub-commands adopted to compartmentalize the UI and code behind it!
I think we should review this further by testing it rather than by further code review. I don't expect this to break things in a way that we don't catch.
@GeorgianaElena maybe you could do a deploy-support and deploy a staging hub before merging though as a precaution, with a health check on the staging before we trigger the full deploy?
Just ran them locally with no issue + a validate command.
Thank you @consideRatio! Merging this now 🚀 |
|
🎉🎉🎉🎉 Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/6570801156 |
Fixes #2024
What the PR does
This PR refactors the deployer's directory structure and creates new typer applications under the main
deployerapp.The commands that we have, then register under these new applications based on their location in the package.
NOTE: the only new code that this PR adds is related to the creation of these typer apps. Other then this, this mostly is about moving files around, renaming them and extracting functions into their own files. More in the commits' description.
Benefits
We are now left with a more clear to follow structure of the main
deployerdirectory instead of having an endless number of python files in a big pool.More, this directory structure maps to the deployer commands, so that it's easier to know where the code that describes a command is.
A note to review more easily
I was very grateful each time @consideRatio split his work into individual logical commits and described what each commit did. That greatly improved my understanding of what the PR did and presented the changes gradually, hence the review was easier <3.
Although I didn't start with this thought in mind because this PR was supposed to be something else and smaller, I did my best to squash the relevant ones together and provide a description for each of them.
So, to review more easily, I suggest going to https://github.com/2i2c-org/infrastructure/pull/3094/commits and look at each commit and its description at a time. Thank you!!!
EDIT: fixes #971