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.

Sign up for GitHub

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

Jump to bottom

Parse terraform template & create new service dialog with parsed input vars #75

Merged
merged 27 commits into from
Oct 1, 2024
Merged

Parse terraform template & create new service dialog with parsed input vars #75

merged 27 commits into from
Oct 1, 2024

Conversation

putmanoj
Copy link
Contributor

@putmanoj putmanoj commented Sep 23, 2024 •
edited by agrare
Loading

  • Parse terraform template, while syncing the git repository
  • Send terraform template to TerraformRunner service, parse for input/output vars
  • In Service Template item, use this parsed input-vars to create textboxes for new Service Dialog.

Dependent:

@putmanoj putmanoj changed the title Parse terraform template for input vars & create new service dialog with the terraform template input vars Parse terraform template & create new service dialog with parsed input vars Sep 23, 2024
agrare
agrare reviewed Sep 26, 2024
View reviewed changes
...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated
before do
ENV["TERRAFORM_RUNNER_URL"] = "https://1.2.3.4:7000"
@hello_world_variables_response = JSON.parse(File.read(File.join(__dir__, "../../../../../lib/terraform/runner/data/responses/hello-world-variables-success.json")))
Copy link
Member

Choose a reason for hiding this comment

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

I think you want File.join(__dir__, "runner/data/responses/hello-world-variables-success.json") here also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't do, different subdirectories in the parent directory tree,
Here only reusing already available data file from another spec

      Failure/Error: @hello_world_variables_response = JSON.parse(File.read(File.join(__dir__, "runner/data/responses/hello-world-variables-success.json")))
      
      Errno::ENOENT:
        No such file or directory @ rb_sysopen - /Users/manoj/git/ManageIQ/manageiq-providers-embedded_terraform/spec/models/manageiq/providers/embedded_terraform/automation_manager/runner/data/responses/hello-world-variables-success.json
      # ./spec/models/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb:26:in `read'

agrare
agrare requested changes Sep 30, 2024
View reviewed changes
...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb Outdated
@@ -1,3 +1,5 @@
TERRAFORM_RUNNER_URL = 'https://1.2.3.4:7000'.freeze
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't add constants to the global scope, a better way to do this is to have a let(:terraform_runner_url) { 'https://1.2.3.4:7000' } inside your RSpec.describe block so it is available to every test in this file but not globally.

Copy link
Member

Choose a reason for hiding this comment

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

E.g. you can see it is causing issues just while running the tests

** ManageIQ master, codename: Spassky
/home/grare/adam/src/manageiq/manageiq-providers-embedded_terraform/spec/models/manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb:1: warning: already initialized constant TERRAFORM_RUNNER_URL
/home/grare/adam/src/manageiq/manageiq-providers-embedded_terraform/spec/lib/terraform/runner_spec.rb:4: warning: previous definition of TERRAFORM_RUNNER_URL was here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

spec/lib/terraform/runner_spec.rb Outdated
@@ -1,12 +1,15 @@
require 'webmock/rspec'
require 'json'

TERRAFORM_RUNNER_URL = "https://1.2.3.4:7000".freeze
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@agrare
Copy link
Member

agrare commented Sep 30, 2024

Re: Revert "fix for codeclient warning - remove
No need to commit a revert to this branch, you can just force push the prior commit

@Fryguy
Copy link
Member

Fryguy commented Sep 30, 2024

Rekicking the specs after merging #76

@Fryguy Fryguy closed this Sep 30, 2024
@Fryguy Fryguy reopened this Sep 30, 2024
@Fryguy Fryguy added the enhancement New feature or request label Sep 30, 2024
@putmanoj
Copy link
Contributor Author

putmanoj commented Oct 1, 2024

Re: Revert "fix for codeclient warning - remove No need to commit a revert to this branch, you can just force push the prior commit

Sure, thanks, will do that

@putmanoj putmanoj force-pushed the create-dialog-for-parsed-template-input-vars branch from eb8bef6 to aacd630 Compare October 1, 2024 05:44
agrare
agrare reviewed Oct 1, 2024
View reviewed changes
app/models/service_template_terraform_template.rb
@@ -17,11 +17,28 @@ def self.create_catalog_item(options, _auth_user)

transaction do
create_from_options(options).tap do |service_template|
dialog_ids = service_template.send(:create_dialogs, config_info)
Copy link
Member

Choose a reason for hiding this comment

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

Why use send here? This method doesn't appear to be private and the method name isn't dynamic so I think you can just

Suggested change
dialog_ids = service_template.send(:create_dialogs, config_info)
dialog_ids = service_template.create_dialogs(config_info)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This came from ansible implementation, myself was puzzled why 'send' ?
simply did not fix what was not broken

Copy link
Member

Choose a reason for hiding this comment

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

👍 That's because their create_dialogs instance method is private, that isn't a good reason to use send but that is why it is needed there and not here.

agrare
agrare reviewed Oct 1, 2024
View reviewed changes
app/models/service_template_terraform_template.rb
opts = super
return unless options.key?(:config_info)

self.class.send(:validate_config_info, opts)
Copy link
Member

Choose a reason for hiding this comment

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

If you need to call this method from an instance variable I'd prefer you make it not a private_class_method and not use send

agrare
agrare reviewed Oct 1, 2024
View reviewed changes
lib/terraform/runner.rb
Comment on lines +192 to +194
def template_variables(
template_path
)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a very long argument list :) can we one-line this?

Suggested change
def template_variables(
template_path
)
def template_variables(template_path)

agrare
agrare reviewed Oct 1, 2024
View reviewed changes
...manageiq/providers/embedded_terraform/automation_manager/configuration_script_source_spec.rb
@@ -15,7 +17,25 @@
let(:repos) { Dir.glob(File.join(repo_dir, "*")) }
let(:repo_dir_structure) { %w[hello_world.tf] }

let(:hello_world_vars_response) do
JSON.parse(File.read(File.join(__dir__, "../../../../../lib/terraform/runner/data/responses/hello-world-variables-success.json")))
Copy link
Member

Choose a reason for hiding this comment

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

You can also use the engine.root to get a path relative to the top of the plugin

Suggested change
JSON.parse(File.read(File.join(__dir__, "../../../../../lib/terraform/runner/data/responses/hello-world-variables-success.json")))
JSON.parse(ManageIQ::Providers::EmbeddedTerraform::Engine.root.join("spec/lib/terraform/runner/data/responses/hello-world-variables-success.json").read)

agrare
agrare approved these changes Oct 1, 2024
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

A few comments mostly about code style so please follow-up with another PR to address these, but nothing blocking functionality so I'm going to merge to get this going

@agrare agrare merged commit 6fd1b36 into ManageIQ:master Oct 1, 2024
3 of 4 checks passed
@putmanoj putmanoj deleted the create-dialog-for-parsed-template-input-vars branch October 1, 2024 13:24
This was referenced Oct 1, 2024
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@putmanoj @agrare @Fryguy