-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
New resource: azurerm_orbital_contact
#19036
Conversation
jiaweitao001
commented
Oct 28, 2022
- New resource to create a contact between ground stations and spacecrafts.
- Acc tests are not runnable now. Due to restrictions of the service team, there're a few steps we need to follow to get the tests running.
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.
LGTM 📡 - just waiting on the service so we can test
Hi @katbyte , just to make sure we are on the same page -- the steps I listed above in the comment are pending HashiCorp's action, it's because the authorized spacecraft must be created through your team's Azure Portal. If you have already submitted the request, please give us a heads up so we can catch up with the service team to see what we can do to enable the ACC tests. Thanks! |
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.
Thanks for this PR - I've taken a look through and left some comments inline, if we can fix those up then we should be able to take another look here.
Thanks!
|
||
"resource_group_name": azure.SchemaResourceGroupName(), | ||
|
||
"spacecraft": { |
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.
is this the Spacecraft ID, if so, can we rename it that?
"spacecraft": { | |
"spacecraft_id": { |
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. Renamed.
ValidateFunc: validation.StringIsNotEmpty, | ||
}, | ||
|
||
"resource_group_name": azure.SchemaResourceGroupName(), |
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.
since the Spacecraft ID includes the Resource Group Name, we could remove this and infer it from the Spacecraft ID?
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.
Right, removed.
ReservationStartTime: props.ReservationStartTime, | ||
ReservationEndTime: props.ReservationEndTime, | ||
GroundStationName: props.GroundStationName, | ||
ContactProfileId: *props.ContactProfile.Id, |
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'll be a crash so we'll need to nil-check this
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.
Added nil check.
if resp, err := client.Delete(ctx, *id); err != nil { | ||
if !response.WasNotFound(resp.HttpResponse) { | ||
return fmt.Errorf("deleting %s: %+v", *id, err) | ||
} | ||
} |
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 would indicate a bug so we should remove this:
if resp, err := client.Delete(ctx, *id); err != nil { | |
if !response.WasNotFound(resp.HttpResponse) { | |
return fmt.Errorf("deleting %s: %+v", *id, err) | |
} | |
} | |
if _, err := client.Delete(ctx, *id); err != nil { | |
return fmt.Errorf("deleting %s: %+v", *id, err) | |
} |
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.
Removed.
524baff
to
dd6ec01
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.
LGTM once tests pass 📡
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.
Hi @katbyte , could you please kindly paste the ID of the spacecraft created here? We will need to make a few modifications to ensure the acc tests can run with it. |
@jiaweitao001 - it should be in teamcity? either way the tests look like something unrelated to the service team?
|
Hi @jiaweitao001. Does this involve using the resource id of an authorised spacecraft? If so, would it make sense to have this input via an env var so anyone with an authorised spacecraft can run the tests? |
@jiaweitao001 - any updates on this one? |
Hi @catriona-m , as we discussed offline weeks ago, there's a request for registering a spacecraft submitted. Could you please check with the service team on how is that going? Thanks! |
@@ -55,7 +55,7 @@ func (r ContactResource) basic(data acceptance.TestData) string { | |||
resource "azurerm_orbital_contact" "test" { | |||
name = "testcontact-%[2]d" | |||
resource_group_name = azurerm_resource_group.test.name | |||
spacecraft_id = azurerm_orbital_spacecraft.test.id | |||
spacecraft_id = "/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/spaceymcspaceface/providers/Microsoft.Orbital/spacecrafts/spaceymcspaceface" |
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.
could we have this id as an env var that gets passed in instead? For example, like these tests have: https://github.com/hashicorp/terraform-provider-azurerm/blob/main/internal/services/bot/bot_channel_facebook_resource_test.go
Tested with spacecraft owned by our team:
|
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.
Thanks for this @jiaweitao001. I've left a couple of minor comments inline but once those get fixed up we can take a look at merging this. Thanks!
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.
Thanks @jiaweitao001 this LGTM!
This functionality has been released in v3.41.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |