fix: add network interface to htcondor execute point module + update …#5225
Conversation
Summary of ChangesHello @Thibaut-Nurit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the HTCondor execute point module by introducing the capability to configure multiple network interfaces for instances, providing greater flexibility in network setup and integration. Concurrently, it updates the version constraints for several core Terraform modules used across the HTCondor components (execute point, access point, and central manager) to allow for more recent module versions, improving maintainability and enabling access to new features or bug fixes from the underlying modules. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for multiple network interfaces in the htcondor-execute-point module, including backward compatibility for the old network configuration. It also updates module dependencies across several htcondor modules and adds the region parameter where it was missing. The changes are well-structured. My review includes suggestions to improve maintainability by using more restrictive version constraints for Terraform modules and to enhance the robustness of the new network interface implementation.
| module "execute_point_instance_template" { | ||
| source = "terraform-google-modules/vm/google//modules/instance_template" | ||
| version = "~> 12.1" | ||
| version = ">= 12.1" |
There was a problem hiding this comment.
Using a permissive version constraint like >= 12.1 can introduce breaking changes from future major versions unexpectedly. It is a best practice to use a more restrictive constraint, such as ~> 12.1, which allows for patch and minor updates but prevents major version bumps (e.g., from 12.x to 13.x). This improves the long-term maintainability and stability of the module.
version = "~> 12.1"
| for network_interface in slice(local.network_interfaces, 1, length(local.network_interfaces)) : { | ||
| network = network_interface.network | ||
| subnetwork = network_interface.subnetwork | ||
| subnetwork_project = var.project_id |
There was a problem hiding this comment.
| module "mig" { | ||
| source = "terraform-google-modules/vm/google//modules/mig" | ||
| version = "~> 12.1" | ||
| version = ">= 12.1" |
There was a problem hiding this comment.
Using a permissive version constraint like >= 12.1 can introduce breaking changes from future major versions unexpectedly. It is a best practice to use a more restrictive constraint, such as ~> 12.1, which allows for patch and minor updates but prevents major version bumps (e.g., from 12.x to 13.x). This improves the long-term maintainability and stability of the module.
version = "~> 12.1"
| module "access_point_instance_template" { | ||
| source = "terraform-google-modules/vm/google//modules/instance_template" | ||
| version = "~> 12.1" | ||
| version = ">= 12.1" |
There was a problem hiding this comment.
Using a permissive version constraint like >= 12.1 can introduce breaking changes from future major versions unexpectedly. It is a best practice to use a more restrictive constraint, such as ~> 12.1, which allows for patch and minor updates but prevents major version bumps (e.g., from 12.x to 13.x). This improves the long-term maintainability and stability of the module.
version = "~> 12.1"
| module "htcondor_ap" { | ||
| source = "terraform-google-modules/vm/google//modules/mig" | ||
| version = "~> 12.1" | ||
| version = ">= 12.1" |
There was a problem hiding this comment.
Using a permissive version constraint like >= 12.1 can introduce breaking changes from future major versions unexpectedly. It is a best practice to use a more restrictive constraint, such as ~> 12.1, which allows for patch and minor updates but prevents major version bumps (e.g., from 12.x to 13.x). This improves the long-term maintainability and stability of the module.
version = "~> 12.1"
| module "central_manager_instance_template" { | ||
| source = "terraform-google-modules/vm/google//modules/instance_template" | ||
| version = "~> 12.1" | ||
| version = ">= 12.1" |
There was a problem hiding this comment.
Using a permissive version constraint like >= 12.1 can introduce breaking changes from future major versions unexpectedly. It is a best practice to use a more restrictive constraint, such as ~> 12.1, which allows for patch and minor updates but prevents major version bumps (e.g., from 12.x to 13.x). This improves the long-term maintainability and stability of the module.
version = "~> 12.1"
| module "htcondor_cm" { | ||
| source = "terraform-google-modules/vm/google//modules/mig" | ||
| version = "~> 12.1" | ||
| version = ">= 12.1" |
There was a problem hiding this comment.
Using a permissive version constraint like >= 12.1 can introduce breaking changes from future major versions unexpectedly. It is a best practice to use a more restrictive constraint, such as ~> 12.1, which allows for patch and minor updates but prevents major version bumps (e.g., from 12.x to 13.x). This improves the long-term maintainability and stability of the module.
version = "~> 12.1"
|
The purpose of this MR is to update the execute point module so it can have multiple network interfaces. Also as the version needed for some cluster toolkit modules is hashicorp/google >= 7, I updated terraform-google-modules/vm/google//modules/mig version so it takes versions >= instead of ~> to stay up to date. |
328f929 to
cdbb68b
Compare
|
/gcbrun |
…htcondor templates version
cdbb68b to
bfca0e0
Compare
|
/gcbrun |
2193e34 to
34ceec0
Compare
34ceec0 to
eac13fa
Compare
…htcondor templates version
Submission Checklist
NOTE: Community submissions can take up to 2 weeks to be reviewed.
Please take the following actions before submitting this pull request.