-
Notifications
You must be signed in to change notification settings - Fork 37
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 support for azure #1043
Add support for azure #1043
Conversation
For commit names use https://www.conventionalcommits.org/en/v1.0.0/ and for lints run |
For editing and rearrange old commits look into |
dc439e6
to
bb47925
Compare
6c61ca5
to
b62ba82
Compare
hydro_deploy/core/src/azure.rs
Outdated
}; | ||
|
||
// #[derive(Debug)] | ||
pub struct LaunchedComputeEngine { |
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.
Should probably be renamed to whatever Azure calls this (Compute Engine is a GCP term?). Also commented line above should be removed.
hydro_deploy/core/src/azure.rs
Outdated
|
||
#[async_trait] | ||
impl LaunchedSSHHost for LaunchedComputeEngine { | ||
fn server_config(&self, bind_type: &ServerStrategy) -> ServerBindConfig { |
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 is nearly identical to GCP, so I wonder if we should move this into the trait and define a separate abstract function for getting the internal_ip
.
hydro_deploy/core/src/azure.rs
Outdated
self.user.as_str() | ||
} | ||
|
||
async fn open_ssh_session(&self) -> Result<AsyncSession<TcpStream>> { |
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.
Same here, this is identical to GCP so we should share the logic.
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 moved the launchedComputeEngine into a shared file
os_type="linux", | ||
machine_size="Standard_B1s", | ||
region="East US", | ||
image="", |
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.
If the image can indeed be omitted, we should make this an optional parameter.
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.
The azure image parameter is a little lengthy, so I made it optional for the user.
deployment = hydro.Deployment() | ||
localhost_machine = deployment.Localhost() | ||
|
||
# gcp_vpc = hydro.GCPNetwork( |
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.
Leftover code
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 two tiny nits, otherwise looks great!
fn get_external_ip(&self) -> Option<String> { | ||
self.external_ip.clone() | ||
} | ||
fn get_internal_ip(&self) -> String { |
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.
nit: newlines between functions
fn get_external_ip(&self) -> Option<String> { | ||
self.external_ip.clone() | ||
} | ||
fn get_internal_ip(&self) -> String { |
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.
nit: newlines between functions
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.
done
…between gcp and azure
ab1b338
to
7eb2e30
Compare
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.
Officially multi cloud! 💪
I accidentally committed some large files, so you won't see the commit history because I copied over the changes onto a fresh clone.
I accidentally committed some large files, so you won't see the commit history because I copied over the changes onto a fresh clone.