Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added app_service_hybrid_connection resource and basic creation test #5330

Closed
wants to merge 13 commits into from
Closed

Added app_service_hybrid_connection resource and basic creation test #5330

wants to merge 13 commits into from

Conversation

jackofallops
Copy link
Member

plenty left to do (docs, test coverage) - but functional on a basic level

@ghost ghost added the size/XL label Jan 7, 2020
katbyte
katbyte requested changes Jan 7, 2020
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @jackofallops, given this a quick preliminary review and its off to a great start with mostly minor comments. Generally if a property is required we should be validating it with at least NoEmptyStrings, and ideally with regex.

azurerm/internal/services/web/registration.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/resource_arm_app_service_hybrid_connection.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/resource_arm_app_service_hybrid_connection.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/resource_arm_app_service_hybrid_connection.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/resource_arm_app_service_hybrid_connection.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/tests/resource_arm_app_service_hybrid_connection_test.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/tests/resource_arm_app_service_hybrid_connection_test.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/tests/resource_arm_app_service_hybrid_connection_test.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/resource_arm_app_service_hybrid_connection.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/resource_arm_app_service_hybrid_connection.go Outdated Show resolved Hide resolved
@katbyte katbyte modified the milestones: v1.40.0, v1.41.0 Jan 7, 2020
katbyte
katbyte requested changes Jan 8, 2020
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the revisions @jackofallops, this is looking pretty good. I still think we should go with relay_id to cater to users of terraform rather then those who may come from the API (we make changes like this now and then)

Other then the remaining minor comments i think this just needs docs & and complete & update tests and it'll be good to merge!

azurerm/internal/services/web/resource_arm_app_service_hybrid_connection.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/resource_arm_app_service_hybrid_connection.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/resource_arm_app_service_hybrid_connection.go Outdated Show resolved Hide resolved
tombuildsstuff
tombuildsstuff reviewed Jan 9, 2020
View reviewed changes
azurerm/internal/services/web/tests/resource_arm_app_service_hybrid_connection_test.go
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
)

func TestAccAzureRMAppServiceHybridConnection_basic(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to add support for & a requires import test here - see other resources for an example (this forms part of #2807)

@jackofallops
Copy link
Member Author

Bug report opened on the API: Azure/azure-rest-api-specs#8108
This blocks completion as property being set by the API is incorrect based on the request, preventing the acceptance test passing.

@jackofallops jackofallops added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Jan 10, 2020
@tombuildsstuff tombuildsstuff modified the milestones: v1.41.0, Blocked Jan 10, 2020
@jackofallops
goimports formatting
b2af635 
renamed attribute relay_arm_uri -> relay_id
wrapped servicebus call for key value in Timeouts
linter fixes
@jackofallops
Added doc page for app service hybrid connection
df8fc24
@tombuildsstuff
Copy link
Member

Since this is currently blocked on a bug in the Azure API - I'm going to close this PR for the moment - however we'll take another look once the upstream issue's been fixed

@jackofallops jackofallops mentioned this pull request Jun 5, 2020
Merged
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2021
@katbyte katbyte deleted the web/hybrid-connection branch May 25, 2021 18:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff left review comments

@katbyte katbyte katbyte requested changes

Assignees
No one assigned
Labels
documentation hashibot/ignore new-resource service/app-service size/XXL upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR
Projects
None yet
Milestone
Blocked
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jackofallops @tombuildsstuff @katbyte