From d976222e246938717b5da19b71942f0faf1aa3a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20D=C4=85bek?= <373530+szemek@users.noreply.github.com> Date: Fri, 23 Sep 2022 09:20:45 +0200 Subject: [PATCH 1/7] Add tests for the same private modules with different versions in single terraform file ``` updater | ERROR No files changed! updater | ERROR /home/dependabot/terraform/lib/dependabot/terraform/file_updater.rb:44:in `updated_dependency_files' updater | ERROR /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:746:in `generate_dependency_files_for' updater | ERROR /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:311:in `check_and_create_pull_request' updater | ERROR /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:109:in `check_and_create_pr_with_error_handling' updater | ERROR /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:80:in `block in run' updater | ERROR /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:80:in `each' updater | ERROR /home/dependabot/dependabot-updater/lib/dependabot/updater.rb:80:in `run' updater | ERROR /home/dependabot/dependabot-updater/lib/dependabot/update_files_job.rb:17:in `perform_job' updater | ERROR /home/dependabot/dependabot-updater/lib/dependabot/base_job.rb:50:in `run' updater | ERROR bin/update_files.rb:22:in `
' ``` --- .../dependabot/terraform/file_updater_spec.rb | 69 +++++++++++++++++++ .../main.tf | 9 +++ 2 files changed, 78 insertions(+) create mode 100644 terraform/spec/fixtures/projects/private_modules_with_different_versions/main.tf diff --git a/terraform/spec/dependabot/terraform/file_updater_spec.rb b/terraform/spec/dependabot/terraform/file_updater_spec.rb index 1d3b98e2945..70c817ac7e9 100644 --- a/terraform/spec/dependabot/terraform/file_updater_spec.rb +++ b/terraform/spec/dependabot/terraform/file_updater_spec.rb @@ -122,6 +122,75 @@ module "s3-webapp" { end end + context "with private modules with different versions" do + let(:project_name) { "private_modules_with_different_versions" } + + let(:dependencies) do + [ + Dependabot::Dependency.new( + name: "example-org-5d3190/s3-webapp/aws", + version: "0.11.0", + previous_version: "0.9.1", + requirements: [{ + requirement: "0.11.0", + groups: [], + file: "main.tf", + source: { + type: "registry", + registry_hostname: "app.terraform.io", + module_identifier: "example-org-5d3190/s3-webapp/aws" + } + }, { + requirement: "0.11.0", + groups: [], + file: "main.tf", + source: { + type: "registry", + registry_hostname: "app.terraform.io", + module_identifier: "example-org-5d3190/s3-webapp/aws" + } + }], + previous_requirements: [{ + requirement: "0.9.1", + groups: [], + file: "main.tf", + source: { + type: "registry", + registry_hostname: "app.terraform.io", + module_identifier: "example-org-5d3190/s3-webapp/aws" + } + }, { + requirement: "0.11.0", + groups: [], + file: "main.tf", + source: { + type: "registry", + registry_hostname: "app.terraform.io", + module_identifier: "example-org-5d3190/s3-webapp/aws" + } + }], + package_manager: "terraform" + ) + ] + end + + it "updates all private modules versions" do + updated_file = subject.find { |file| file.name == "main.tf" } + + expect(updated_file.content).to include(<<~HCL) + module "s3-webapp" { + source = "app.terraform.io/example-org-5d3190/s3-webapp/aws" + version = "0.11.0" + } + + module "s3-webapp" { + source = "app.terraform.io/example-org-5d3190/s3-webapp/aws" + version = "0.11.0" + } + HCL + end + end + context "with a private provider" do let(:project_name) { "private_provider" } diff --git a/terraform/spec/fixtures/projects/private_modules_with_different_versions/main.tf b/terraform/spec/fixtures/projects/private_modules_with_different_versions/main.tf new file mode 100644 index 00000000000..3d7ba8f24f1 --- /dev/null +++ b/terraform/spec/fixtures/projects/private_modules_with_different_versions/main.tf @@ -0,0 +1,9 @@ +module "s3-webapp" { + source = "app.terraform.io/example-org-5d3190/s3-webapp/aws" + version = "0.11.0" +} + +module "s3-webapp" { + source = "app.terraform.io/example-org-5d3190/s3-webapp/aws" + version = "0.9.1" +} From 232c70030b19b9595aef926cf224b900b40f96cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20D=C4=85bek?= <373530+szemek@users.noreply.github.com> Date: Fri, 23 Sep 2022 09:26:49 +0200 Subject: [PATCH 2/7] Use gsub! instead of sub! for replacing all matching occurrences --- terraform/lib/dependabot/terraform/file_updater.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/lib/dependabot/terraform/file_updater.rb b/terraform/lib/dependabot/terraform/file_updater.rb index cbeec8d8f4b..582622ed46f 100644 --- a/terraform/lib/dependabot/terraform/file_updater.rb +++ b/terraform/lib/dependabot/terraform/file_updater.rb @@ -89,7 +89,7 @@ def update_git_declaration(new_req, old_req, updated_content, filename) def update_registry_declaration(new_req, old_req, updated_content) regex = new_req[:source][:type] == "provider" ? provider_declaration_regex : registry_declaration_regex - updated_content.sub!(regex) do |regex_match| + updated_content.gsub!(regex) do |regex_match| regex_match.sub(/^\s*version\s*=.*/) do |req_line_match| req_line_match.sub(old_req[:requirement], new_req[:requirement]) end From 8c27c93eaa37e7a686c7642599bd2fa94311b6fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20D=C4=85bek?= <373530+szemek@users.noreply.github.com> Date: Fri, 23 Sep 2022 12:50:29 +0200 Subject: [PATCH 3/7] Use symmetric difference to detect requirement_changed? --- common/lib/dependabot/file_updaters/base.rb | 3 ++- terraform/spec/dependabot/terraform/file_updater_spec.rb | 4 ++-- .../projects/private_modules_with_different_versions/main.tf | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/common/lib/dependabot/file_updaters/base.rb b/common/lib/dependabot/file_updaters/base.rb index 90f7fe36086..a16874a44b0 100644 --- a/common/lib/dependabot/file_updaters/base.rb +++ b/common/lib/dependabot/file_updaters/base.rb @@ -41,7 +41,8 @@ def file_changed?(file) def requirement_changed?(file, dependency) changed_requirements = - dependency.requirements - dependency.previous_requirements + dependency.requirements - dependency.previous_requirements | + dependency.previous_requirements - dependency.requirements changed_requirements.any? { |f| f[:file] == file.name } end diff --git a/terraform/spec/dependabot/terraform/file_updater_spec.rb b/terraform/spec/dependabot/terraform/file_updater_spec.rb index 70c817ac7e9..c686052b1dc 100644 --- a/terraform/spec/dependabot/terraform/file_updater_spec.rb +++ b/terraform/spec/dependabot/terraform/file_updater_spec.rb @@ -178,12 +178,12 @@ module "s3-webapp" { updated_file = subject.find { |file| file.name == "main.tf" } expect(updated_file.content).to include(<<~HCL) - module "s3-webapp" { + module "s3-webapp-first" { source = "app.terraform.io/example-org-5d3190/s3-webapp/aws" version = "0.11.0" } - module "s3-webapp" { + module "s3-webapp-second" { source = "app.terraform.io/example-org-5d3190/s3-webapp/aws" version = "0.11.0" } diff --git a/terraform/spec/fixtures/projects/private_modules_with_different_versions/main.tf b/terraform/spec/fixtures/projects/private_modules_with_different_versions/main.tf index 3d7ba8f24f1..7a04e89b4ec 100644 --- a/terraform/spec/fixtures/projects/private_modules_with_different_versions/main.tf +++ b/terraform/spec/fixtures/projects/private_modules_with_different_versions/main.tf @@ -1,9 +1,9 @@ -module "s3-webapp" { +module "s3-webapp-first" { source = "app.terraform.io/example-org-5d3190/s3-webapp/aws" version = "0.11.0" } -module "s3-webapp" { +module "s3-webapp-second" { source = "app.terraform.io/example-org-5d3190/s3-webapp/aws" version = "0.9.1" } From 7e0d7864d1a09a42ee10c646e642dd9db05f0029 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20D=C4=85bek?= <373530+szemek@users.noreply.github.com> Date: Fri, 23 Sep 2022 14:29:13 +0200 Subject: [PATCH 4/7] Override requirement_changed? in terraform subclass --- common/lib/dependabot/file_updaters/base.rb | 3 +-- terraform/lib/dependabot/terraform/file_updater.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/common/lib/dependabot/file_updaters/base.rb b/common/lib/dependabot/file_updaters/base.rb index a16874a44b0..90f7fe36086 100644 --- a/common/lib/dependabot/file_updaters/base.rb +++ b/common/lib/dependabot/file_updaters/base.rb @@ -41,8 +41,7 @@ def file_changed?(file) def requirement_changed?(file, dependency) changed_requirements = - dependency.requirements - dependency.previous_requirements | - dependency.previous_requirements - dependency.requirements + dependency.requirements - dependency.previous_requirements changed_requirements.any? { |f| f[:file] == file.name } end diff --git a/terraform/lib/dependabot/terraform/file_updater.rb b/terraform/lib/dependabot/terraform/file_updater.rb index 582622ed46f..f524b7bed6f 100644 --- a/terraform/lib/dependabot/terraform/file_updater.rb +++ b/terraform/lib/dependabot/terraform/file_updater.rb @@ -46,6 +46,14 @@ def updated_dependency_files updated_files end + def requirement_changed?(file, dependency) + changed_requirements = + (dependency.requirements - dependency.previous_requirements) | + (dependency.previous_requirements - dependency.requirements) + + changed_requirements.any? { |f| f[:file] == file.name } + end + private def updated_terraform_file_content(file) From 0dddd9e0f48b6090330c0f87f0029c85cca25a8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20D=C4=85bek?= <373530+szemek@users.noreply.github.com> Date: Fri, 23 Sep 2022 14:48:18 +0200 Subject: [PATCH 5/7] Move requirement_changed? to private methods --- terraform/lib/dependabot/terraform/file_updater.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/terraform/lib/dependabot/terraform/file_updater.rb b/terraform/lib/dependabot/terraform/file_updater.rb index f524b7bed6f..20c5e162940 100644 --- a/terraform/lib/dependabot/terraform/file_updater.rb +++ b/terraform/lib/dependabot/terraform/file_updater.rb @@ -46,6 +46,8 @@ def updated_dependency_files updated_files end + private + def requirement_changed?(file, dependency) changed_requirements = (dependency.requirements - dependency.previous_requirements) | @@ -54,8 +56,6 @@ def requirement_changed?(file, dependency) changed_requirements.any? { |f| f[:file] == file.name } end - private - def updated_terraform_file_content(file) content = file.content.dup From 953fe87b591a157b081f3808c24e132cceaea372 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20D=C4=85bek?= <373530+szemek@users.noreply.github.com> Date: Thu, 29 Sep 2022 23:57:57 +0200 Subject: [PATCH 6/7] Add an explanation for overwritten method --- terraform/lib/dependabot/terraform/file_updater.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/terraform/lib/dependabot/terraform/file_updater.rb b/terraform/lib/dependabot/terraform/file_updater.rb index 20c5e162940..feddfd1b9b3 100644 --- a/terraform/lib/dependabot/terraform/file_updater.rb +++ b/terraform/lib/dependabot/terraform/file_updater.rb @@ -48,6 +48,20 @@ def updated_dependency_files private + # Terraform allows to use a module from the same source multiple times + # To detect any changes in dependencies we need to overwrite an implementation from the base class + # + # Example (for simplicity other parameters are skipped): + # previous_requirements = [{requirement: "0.9.1"}, {requirement: "0.11.0"}] + # requirements = [{requirement: "0.11.0"}, {requirement: "0.11.0"}] + # + # Simple difference between arrays gives: + # requirements - previous_requirements = [] + # which loses an information that one of our requirements has changed. + # + # By using symmetric difference: + # (requirements - previous_requirements) | (previous_requirements - requirements) = [{requirement: "0.9.1"}] + # we can detect that change. def requirement_changed?(file, dependency) changed_requirements = (dependency.requirements - dependency.previous_requirements) | From 0ad52d07655549e89af911dcd930d10c6333f0cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20D=C4=85bek?= <373530+szemek@users.noreply.github.com> Date: Fri, 30 Sep 2022 00:07:35 +0200 Subject: [PATCH 7/7] Small adjustments in provided example --- terraform/lib/dependabot/terraform/file_updater.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/terraform/lib/dependabot/terraform/file_updater.rb b/terraform/lib/dependabot/terraform/file_updater.rb index feddfd1b9b3..c2a81df7ab8 100644 --- a/terraform/lib/dependabot/terraform/file_updater.rb +++ b/terraform/lib/dependabot/terraform/file_updater.rb @@ -52,15 +52,17 @@ def updated_dependency_files # To detect any changes in dependencies we need to overwrite an implementation from the base class # # Example (for simplicity other parameters are skipped): - # previous_requirements = [{requirement: "0.9.1"}, {requirement: "0.11.0"}] + # previous_requirements = [{requirement: "0.9.1"}, {requirement: "0.11.0"}] # requirements = [{requirement: "0.11.0"}, {requirement: "0.11.0"}] # # Simple difference between arrays gives: - # requirements - previous_requirements = [] + # requirements - previous_requirements + # => [] # which loses an information that one of our requirements has changed. # # By using symmetric difference: - # (requirements - previous_requirements) | (previous_requirements - requirements) = [{requirement: "0.9.1"}] + # (requirements - previous_requirements) | (previous_requirements - requirements) + # => [{requirement: "0.9.1"}] # we can detect that change. def requirement_changed?(file, dependency) changed_requirements =