Sync Terraform Templates from git repo #5

putmanoj · 2024-03-19T10:13:57Z

Sync Terraform Template

find directories containing .tf/.tf.json files in git repo worktree
add the relative-path as Template name(path)
add basic spec to verify ConfigurationScriptSource

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb

app/models/manageiq/providers/embedded_terraform/automation_manager/template.rb

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb

agrare · 2024-03-19T13:50:11Z

@putmanoj Update app/models/manageiq/providers/embedded_terraform/automation_ma…nager/configuration_script_source.rb isn't a very helpful commit message and having 6 commits with the same commit message is confusing. If you're fixing rubocop warnings or other small items I like to rebase and amend the commit which introduced the issue rather than adding a new commit.

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb

agrare · 2024-03-19T17:34:56Z

TODO before merge:

Spec tests covering ConfigurationScriptSource sync
Fix commits per Sync Terraform Templates from git repo #5 (comment)

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb

jrafanie · 2024-03-25T17:35:46Z

@putmanoj I added some tests and made a few changes to the existing ones on my fork: master...jrafanie:manageiq-providers-embedded_terraform:git-sync-templates

you can cherry-pick them or I can help you bring them over. Let me know what you think.

putmanoj · 2024-03-26T04:11:03Z

@putmanoj I added some tests and made a few changes to the existing ones on my fork: master...jrafanie:manageiq-providers-embedded_terraform:git-sync-templates

you can cherry-pick them or I can help you bring them over. Let me know what you think.

Thanks, will cherry-pick.

Fryguy · 2024-03-26T15:03:01Z

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb

+
+    # checkout files to temp dir, we need for parsing input/output vars
+    git_checkout_tempdir = Dir.mktmpdir("terraform-git")
+    checkout_git_repository(git_checkout_tempdir)


Technically this isn't necessary - everything should be available from the worktree. However, I admit that working with rugged objects is complicated, so this is fine for now.

Also will this checkout happen on every refresh? I feel like that could be expensive. 🤔

Is the update or the checkout expensive? Maybe just doing the checkout would be ok if it's already cloned.

checkout can be expensive. Under the covers we do bare repos, so it's literally having to do I/O for all of the files each time. This is why straight Rugged is better, because it's all in-memory and doesn't require creating a file.

@putmanoj can you review this and comment here? Thanks.

~~TODO use GitWorktree~~

Fryguy · 2024-03-26T15:03:50Z

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb

+        next unless filepath.end_with?(".tf", ".tf.json")
+
+        parent_dir = File.dirname(filepath)
+        name = ManageIQ::Providers::EmbeddedTerraform::AutomationManager::ConfigurationScriptSource.template_name_from_git_repo_url(


Suggested change

name = ManageIQ::Providers::EmbeddedTerraform::AutomationManager::ConfigurationScriptSource.template_name_from_git_repo_url(

name = self.class.template_name_from_git_repo_url(

Should be resolved

Fryguy · 2024-03-26T15:04:26Z

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb

+        next if template_dirs.key?(name)
+
+        full_path = File.join(git_checkout_tempdir, parent_dir)
+        _log.info("Local full path : #{full_path}")


Do we need this line? I'm concerned it will flood the logs. Perhaps make it debug?

Should be resolved

Fryguy · 2024-03-26T15:05:01Z

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb

+        _log.info("Local full path : #{full_path}")
+        files = Dir.children(full_path)
+
+        # :TODO add parsing for input/output vars


Suggested change

# :TODO add parsing for input/output vars

# TODO: add parsing for input/output vars

Should be resolved

Fryguy · 2024-03-26T15:08:27Z

app/models/manageiq/providers/embedded_terraform/provider/default_terraform_objects.rb

+  def get_default_terraform_object(name)
+    default_terraform_objects.find_by(:name => name).try(:value).try(:to_i)
+  end
+
+  def set_default_terraform_object(name, value)
+    default_terraform_objects.find_or_initialize_by(:name => name).update(:value => value)
+  end


These helper methods don't see to be buying anything here - prefer just inlining these methods directly into the callers

(In general, in Ruby, we avoid get_ and set_ methods when a proper getter/setter method like you have on lines 10 and 14 will suffice)

Fryguy · 2024-03-26T15:09:08Z

app/models/manageiq/providers/embedded_terraform/provider.rb

@@ -0,0 +1,20 @@
+class ManageIQ::Providers::EmbeddedTerraform::Provider < Provider
+  include DefaultTerraformObjects


I'm not sure I understand why this is a module because I don't see it reused anywhere (or do you plan to reuse in the future? Prefer inlining the methods from the module directly here unless you plan on reuse.

Needs review.

Fryguy · 2024-03-26T15:10:04Z

app/models/manageiq/providers/embedded_terraform/provider/default_terraform_objects.rb

@@ -0,0 +1,27 @@
+module ManageIQ::Providers::EmbeddedTerraform::Provider::DefaultTerraformObjects


What is the purpose of these default_terraform_objects with respect to syncing terraform templates? I don't see any callers in this PR.

I think this is copy&paste from embedded ansible, it isn't necessary here and should be deleted https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/embedded_ansible/provider/default_ansible_objects.rb

Needs review.

Fryguy · 2024-03-26T15:11:33Z

@putmanoj Is this built on top of another PR? If so can you indicate that in the opening post? It's hard to follow with 24 commits.

spec/support/evm_spec_helper.rb

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb

agrare · 2024-03-27T13:19:10Z

@putmanoj please rebase on master rather than creating a new merge commit in this branch as it keeps the history cleaner

Merge branch 'git-sync-templates' of github.com:putmanoj/manageiq-providers-embedded_terraform into git-sync-templates
d10963e

agrare · 2024-03-27T13:24:16Z

app/models/manageiq/providers/embedded_terraform/provider.rb

@@ -0,0 +1,20 @@
+class ManageIQ::Providers::EmbeddedTerraform::Provider < Provider


I'm not sure we need an EmbeddedTerraform::Provider, this looks like another copy&paste from EmbeddedAnsible

Needs review.

I removed the included DefaultTerraformObjects module. It looks like the tests are creating provider/managers through create_in_provider, and perhaps that's why we have a provider.rb with code that does ensure_managers. I'm not sure how it's expected to work without a provider.

I removed Provider and fixed the factories and specs.

This was copied&pasted from EmbeddedAnsible, but EmbeddedAnsible seeds a Provider [ref] where EmbeddedTerraform does not [ref]. This meant that these specs did not match the behavior of how the embedded_terraform manager was being created.

agrare · 2024-03-27T13:25:37Z

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb

@@ -0,0 +1,518 @@
+FakeTerraformRepo = Spec::Support::FakeAnsibleRepo # TODO: replace FakeAnsibleRepo with FakeTerraformRepo


FYI here is an example of a "pluggable" support repo class https://github.com/ManageIQ/manageiq-providers-workflows/blob/master/spec/support/fake_workflows_repo.rb

Should be resolved

spec/support/evm_spec_helper.rb

agrare · 2024-03-27T19:38:20Z

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb

+      repo.git_branch_create("other_branch")
+      stub_const("GitRepository::GIT_REPO_DIRECTORY", repo_dir)
+
+      EvmSpecHelper.assign_embedded_terraform_role


Suggested change

EvmSpecHelper.assign_embedded_terraform_role

EvmSpecHelper.assign_role("embedded_terraform")

Should be resolved

agrare · 2024-03-27T19:40:28Z

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb

+          expect(result).to be_an(described_class)
+          expect(result.scm_type).to eq("git")
+          expect(result.scm_branch).to eq("master")
+          expect(result.status).to eq("successful")
+          expect(result.last_updated_on).to be_an(Time)
+          expect(result.last_update_error).to be_nil


NOTE you can use have_attributes to check multiple attributes at once:

expect(result).to have_attributes( :scm_type => "git", :scm_branch => "master" ... )

Should be resolved

agrare · 2024-03-27T19:40:59Z

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb

+
+          result = described_class.create_in_provider(manager.id, params)
+
+          expect(result).to be_an(described_class)


Small grammar fix

Suggested change

expect(result).to be_an(described_class)

expect(result).to be_a(described_class)

Should be resolved

agrare · 2024-03-27T19:43:08Z

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb

+          expect(result.scm_type).to eq("git")
+          expect(result.scm_branch).to eq("master")
+          expect(result.status).to eq("successful")
+          expect(result.last_updated_on).to be_an(Time)


You could probably use be_within to check the actual timestamp here: expect(result.last_updated_on).to be_within(2.seconds).of(Time.now.utc)

Should be resolved

jrafanie · 2024-03-28T00:23:50Z

@putmanoj Ok, I think I resolved all of the comments with my 👍 in them. I think everything but provider.rb. I think that needs to be resolved. Let's remove that module and anything in there we don't need yet. We can add it later.

Please review my branch: at master...jrafanie:manageiq-providers-embedded_terraform:git-sync-templates

I squashed our fixup commits. My final commits can be combined once you review them. Note, I really deleted a lot of tests because they still passed when I commented out most of the configuration source class so they're not really testing it.

I didn't want to push directly to your branch here but mine is correctly tracking upstream here so if you want to use my branch, we can push that.

putmanoj · 2024-03-28T06:42:50Z

Please review my branch: at master...jrafanie:manageiq-providers-embedded_terraform:git-sync-templates

LGTM

jrafanie · 2024-03-28T13:35:28Z

ok, @putmanoj, I force pushed to your fork. I've addressed all concerns not yet addressed other than the questions about provider.rb including DefaultTerraformObjects from Jason and Adam. Actually, I think Adam said you might not even need provider.rb here. Other than that, we need to set the role properly for the queue items. I marked those commented tests as pending so we can properly set the role. I assume we want to use the new server role. I'm not sure what else is needed. Can you finish up those things and we can get ready for merge and then I can help get your other work rebased and tested? Thanks!

Add relative-path of dir's with .tf/.tf.json files Add specs for git sync * add spec for .create_in_provider * Add EmbeddedTerraform Provider class * add ensure_managers * fixed .create_in_provider test * Use basename of dir, as template-name pre-fix, & full git repo url details as suffix * Add tests for: templates-in-repo update-in-provider update_in_provider_queue delete_in_provider_queue

* Add tests to describe behavior of template_name_from_git_repo_url * Drop templates_for, test both name and payload * We should test the payload contents are correct in addition to the name. * Drop redundant test since we're testing names and payloads * Add test verifying no longer existing payloads are removed on sync * Mark pending tests where we need to set the role in the queue * Create a fake terraform git repo for test

I changed configuration_script_source.rb to: ```ruby class ManageIQ::Providers::EmbeddedTerraform::AutomationManager::ConfigurationScriptSource < ManageIQ::Providers::EmbeddedAutomationManager::ConfigurationScriptSource FRIENDLY_NAME = "Embedded Terraform Repository".freeze def sync end end ``` and all of the removed tests still pass which tells me either the tests aren't needed, the code needs to be implemented and/or the test needs to test what unique behavior is provided by this class.

jrafanie · 2024-03-28T18:27:37Z

Note, #9 was extracted from here

agrare · 2024-03-29T13:24:09Z

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb

+  private_class_method def self.queue_role
+    "embedded_terraform"
+  end
+


I added this to fix the three pending specs in configuration_script_source_spec

kbrock · 2024-04-01T15:35:42Z

I like the changes introduced by Adam and the other code looks good (from a person who doesn't use rugged enough)

LGTM

jrafanie reviewed Mar 19, 2024

View reviewed changes

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb Outdated Show resolved Hide resolved

jrafanie reviewed Mar 19, 2024

View reviewed changes

app/models/manageiq/providers/embedded_terraform/automation_manager/template.rb Outdated Show resolved Hide resolved

agrare reviewed Mar 19, 2024

View reviewed changes

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb Outdated Show resolved Hide resolved

agrare reviewed Mar 19, 2024

View reviewed changes

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb Outdated Show resolved Hide resolved

agrare reviewed Mar 19, 2024

View reviewed changes

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb Outdated Show resolved Hide resolved

agrare reviewed Mar 19, 2024

View reviewed changes

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb Show resolved Hide resolved

agrare reviewed Mar 19, 2024

View reviewed changes

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb Outdated Show resolved Hide resolved

agrare reviewed Mar 19, 2024

View reviewed changes

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb Outdated Show resolved Hide resolved

agrare reviewed Mar 19, 2024

View reviewed changes

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb Outdated Show resolved Hide resolved

agrare added the enhancement New feature or request label Mar 19, 2024

jrafanie reviewed Mar 21, 2024

View reviewed changes

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated Show resolved Hide resolved

jrafanie reviewed Mar 21, 2024

View reviewed changes

...dels/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source.rb Show resolved Hide resolved

jrafanie reviewed Mar 21, 2024

View reviewed changes

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated Show resolved Hide resolved

jrafanie reviewed Mar 21, 2024

View reviewed changes

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated Show resolved Hide resolved

Fryguy mentioned this pull request Mar 26, 2024

Create an Embedded Terraform provider ManageIQ/manageiq#22956

Closed

40 tasks

Fryguy reviewed Mar 26, 2024

View reviewed changes

jrafanie reviewed Mar 26, 2024

View reviewed changes

spec/support/evm_spec_helper.rb Outdated Show resolved Hide resolved

jrafanie reviewed Mar 26, 2024

View reviewed changes

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated Show resolved Hide resolved

jrafanie reviewed Mar 26, 2024

View reviewed changes

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated Show resolved Hide resolved

jrafanie reviewed Mar 26, 2024

View reviewed changes

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated Show resolved Hide resolved

jrafanie reviewed Mar 26, 2024

View reviewed changes

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated Show resolved Hide resolved

jrafanie reviewed Mar 26, 2024

View reviewed changes

...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated Show resolved Hide resolved

agrare reviewed Mar 27, 2024

View reviewed changes

spec/support/evm_spec_helper.rb Outdated Show resolved Hide resolved

Fryguy assigned agrare Mar 27, 2024

agrare reviewed Mar 27, 2024

View reviewed changes

jrafanie force-pushed the git-sync-templates branch 2 times, most recently from 4f3d599 to f8ad986 Compare March 28, 2024 13:28

jrafanie force-pushed the git-sync-templates branch from f8ad986 to 78e58f6 Compare March 28, 2024 13:40

putmanoj and others added 3 commits March 28, 2024 14:18

jrafanie force-pushed the git-sync-templates branch from 78e58f6 to bf0fd97 Compare March 28, 2024 18:19

jrafanie and others added 3 commits March 28, 2024 16:32

Drop DefaultTerraformObjects since we don't need it yet

2df5401

Set role=embedded_terraform for repo queue actions

2104eff

Remove Unused EmbeddedTerraform::Provider

0c8bcae

agrare reviewed Mar 29, 2024

View reviewed changes

Fix spacing linter warnings

421d6d3

kbrock merged commit ed4c9a1 into ManageIQ:master Apr 1, 2024
3 of 4 checks passed

	name = ManageIQ::Providers::EmbeddedTerraform::AutomationManager::ConfigurationScriptSource.template_name_from_git_repo_url(
	name = self.class.template_name_from_git_repo_url(

	# :TODO add parsing for input/output vars
	# TODO: add parsing for input/output vars

		@@ -0,0 +1,20 @@
		class ManageIQ::Providers::EmbeddedTerraform::Provider < Provider
		include DefaultTerraformObjects

		@@ -0,0 +1,27 @@
		module ManageIQ::Providers::EmbeddedTerraform::Provider::DefaultTerraformObjects

		@@ -0,0 +1,518 @@
		FakeTerraformRepo = Spec::Support::FakeAnsibleRepo # TODO: replace FakeAnsibleRepo with FakeTerraformRepo

	EvmSpecHelper.assign_embedded_terraform_role
	EvmSpecHelper.assign_role("embedded_terraform")


		result = described_class.create_in_provider(manager.id, params)

		expect(result).to be_an(described_class)

	expect(result).to be_an(described_class)
	expect(result).to be_a(described_class)

Sync Terraform Templates from git repo #5

Sync Terraform Templates from git repo #5

Conversation

putmanoj commented Mar 19, 2024 • edited Loading

agrare commented Mar 19, 2024

agrare commented Mar 19, 2024 • edited Loading

jrafanie commented Mar 25, 2024

putmanoj commented Mar 26, 2024

Fryguy Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Mar 26, 2024

agrare commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Mar 28, 2024

putmanoj commented Mar 28, 2024

jrafanie commented Mar 28, 2024

jrafanie commented Mar 28, 2024

Choose a reason for hiding this comment

kbrock commented Apr 1, 2024

putmanoj commented Mar 19, 2024 •

edited

Loading

agrare commented Mar 19, 2024 •

edited

Loading

Fryguy Mar 26, 2024 •

edited

Loading

agrare Mar 29, 2024 •

edited

Loading