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.

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 Terraform module to configure Azure Batch easily #1

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

rchretien
Copy link

Summary

Terraform module that encapsulates Azure Batch's configuration with Docker-compatible nodes running a custom Docker image hosted on a private registry.

The usage is indicated as comment in the main.tf file hosted in the root directory.

Future improvement

  • Make the number of default nodes dependent on the environment.
  • Make the autoscale formula dependent on the environment.

Copy link
Member

@lsorber lsorber 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 @rchretien! Please find some comments below.

azure-batch-module/main.tf Outdated Show resolved Hide resolved
azure-batch-module/main.tf Outdated Show resolved Hide resolved
azure-batch-module/main.tf Outdated Show resolved Hide resolved
azure-batch-module/main.tf Outdated Show resolved Hide resolved
azure-batch-module/main.tf Outdated Show resolved Hide resolved
azure-batch-module/main.tf Outdated Show resolved Hide resolved
azure-batch-module/main.tf Outdated Show resolved Hide resolved
azure-batch-module/main.tf Outdated Show resolved Hide resolved
azure-batch-module/modules/batch-account/main.tf Outdated Show resolved Hide resolved
azure-batch-module/main.tf Outdated Show resolved Hide resolved
@lsorber lsorber added the enhancement New feature or request label Mar 29, 2022
Copy link
Author

@rchretien rchretien left a comment

Choose a reason for hiding this comment

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

Here's the new version of the PR.

Providers' requirements are specified in a separate file called versions.tf and different outputs are now indicated (i.e. the pool_id, the login_server of the container registry, as well as the admin username/password) in the outputs.tf file.

The corpus of the module is also regrouped in the root folder.

Normally, the module is usable "as such", although one has to provide his own subscription ID as a parameter of the module (I don't how we can circumvent it).

Copy link
Member

@lsorber lsorber left a comment

Choose a reason for hiding this comment

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

The module looks very good, thanks @rchretien! Only some details left now.

azure_batch/main.tf Outdated Show resolved Hide resolved
azure_batch/main.tf Outdated Show resolved Hide resolved
azure_batch/main.tf Show resolved Hide resolved
azure_batch/main.tf Outdated Show resolved Hide resolved
azure_batch/main.tf Outdated Show resolved Hide resolved
azure_batch/main.tf Outdated Show resolved Hide resolved
azure_batch/main.tf Outdated Show resolved Hide resolved
azure_batch/main.tf Outdated Show resolved Hide resolved
azure_batch/main.tf Outdated Show resolved Hide resolved
azure_batch/main.tf Outdated Show resolved Hide resolved
@rchretien
Copy link
Author

rchretien commented Aug 6, 2022

Thanks @lsorber for your comments and for the review!

Here's the new version of the PR.

Every suggestion has been accepted.

I changed a bit my mind based on your comments and have therefore removed anything related to the creation of a private container registry or to the push of a Docker image from the module definition. The information (login_server, username and password) of the container registry (either pre-existing or created in a previous step of a CI/CD pipeline) is now provided as input variables. I think it can now smoothly integrate within a pipeline.

Should I add a README?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants