Azure / Synapse-workspace-deployment

MIT License
27 stars 36 forks source link

Treat AutoResolveIntegrationRuntime as a default artifact #26

Closed owenfarrell closed 2 years ago

owenfarrell commented 3 years ago

When creating a new Synapse Workspace, an Integration Runtime named AutoResolveIntegrationRuntime is automatically created.

The workspace manages the integration runtime in Azure to connect to required data source/destination or external compute in public network. The compute resource is elastic allocated based on performance requirement of activities.

owenfarrell commented 2 years ago

@lordozb - Thoughts?

lordozb commented 2 years ago

Hello @owenfarrell Sorry for delay in getting back.

Changes will also be required in Here because integration runtime is of type Microsoft.Synapse/workspaces/integrationRuntimes.

Can you please also add some positive and negative unit tests Here

owenfarrell commented 2 years ago

Changes will also be required in Here because integration runtime is of type Microsoft.Synapse/workspaces/integrationRuntimes.

When I took a look at this method, the logic was starting to get a bit gnarly. So in order to help make future maintenance easier, I refactored that method to use enumerated instances of a class instead of three independent arrays.

Can you please also add some positive and negative unit tests Here

I also renamed some of the test data constants. When I first looked at them, I got really confused how/why each object existed since most of them were named DEFAULT.... When I realized that some of them were intended to represent custom artifacts, it made a bit more sense.

lordozb commented 2 years ago

Hello @owenfarrell Thank you for contributing to the project. We have asked our partner team which manages integration runtime to let us know if skipping autoresolve IR will cause any issue or not. We are waiting for their response.

owenfarrell commented 2 years ago

@lordozb - What's the verdict?

lordozb commented 2 years ago

Hello @owenfarrell Apologies for delay in getting back. We can go ahead and mark it as a default artifact. However, due to the recent commit there has been some merge conflicts. If you can resolve the conflicts I can go ahead and merge the PR. If you are unavailable I can resolve the conflicts and merge your PR.

owenfarrell commented 2 years ago

@lordozb - I can resolve the conflicts. But I have a couple questions about #28.

  1. Did you just copy the DefaultArtifact class that I added here and paste it in to your PR? Or is there something genuinely different?
  2. Is there a way to selectively disable the deployment of managed private endpoints? Your PR (as written) seems to duplicate some automation capabilities that exist elsewhere (namely https://github.com/hashicorp/terraform-provider-azurerm/pull/9260). And Terraform is adding more a la carte capabilities for Synapse artifacts, courtesy of @ms-henglu. I guess my question really is: is this CI/CD plugin trying to complement other automation tools? or compete with them?
lordozb commented 2 years ago

@owenfarrell

  1. It is mostly the same. I reused it to keep the changes required to merge this PR as min as possible. I have added 3 more default artifacts. Apart from that in the matches method I have replaced the indexOf call with name.toLowerCase().includes(this.name.toLowerCase()). You can either add autoresolve IR to this class and delete the file you created or you can add the new default artifacts to the file you created and remove the class from here.
  2. Terraform does not supports all azure resources and also not all Synapse customers use terraform. So we have to support a separate CICD plugin for our customers.
owenfarrell commented 2 years ago

@lordozb - so back to my question.... is there a way to toggle off the deployment of MPEs (which do have alternative automation options)?

lordozb commented 2 years ago

@owenfarrell Currently, the code deploys all the artifacts in the template. There is no way to do selective deployment as of now.

owenfarrell commented 2 years ago

Closing this PR since the code was copied/committed elsewhere