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

Add methods to fetch terraform stdout for showing in UI. #78

Merged
merged 16 commits into from
Oct 21, 2024
Merged

Add methods to fetch terraform stdout for showing in UI. #78

merged 16 commits into from
Oct 21, 2024

Conversation

putmanoj
Copy link
Contributor

@putmanoj putmanoj commented Oct 16, 2024 •
edited by agrare
Loading

Related:

Add methods in Stack, to fetch terraform job stdout from TerraformRunner api/stack

@putmanoj putmanoj mentioned this pull request Oct 16, 2024
Merged
@putmanoj putmanoj changed the title Add method fetch terraform stdout for UI Add methods to fetch terraform stdout for UI Oct 16, 2024
@putmanoj putmanoj mentioned this pull request Oct 16, 2024
Closed
@Fryguy
Copy link
Member

Fryguy commented Oct 16, 2024

@putmanoj Can you add some specs for these methods? You can probably copy a lot from core's spec/models/manageiq/providers/embedded_ansible/automation_manager/job_spec.rb around line 143+

@putmanoj
Copy link
Contributor Author

@putmanoj Can you add some specs for these methods? You can probably copy a lot from core's spec/models/manageiq/providers/embedded_ansible/automation_manager/job_spec.rb around line 143+

specs added

@putmanoj putmanoj changed the title Add methods to fetch terraform stdout for UI Add methods to fetch terraform stdout for showing in UI. Oct 19, 2024
agrare
agrare reviewed Oct 21, 2024
View reviewed changes
spec/models/manageiq/providers/embedded_terraform/automation_manager/stack_spec.rb
Comment on lines +47 to +51
ManageIQ::Providers::EmbeddedTerraform::AutomationManager::Job.create_job(template, {}, {}, []).tap do |job|
job.state = "finished"
job.options = {
:terraform_stack_id => hello_world_retrieve_response['stack_id']
}
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 a FactoryBot factory would be a better way to set up these Job objects

Copy link
Member

Choose a reason for hiding this comment

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

#80

agrare
agrare reviewed Oct 21, 2024
View reviewed changes
spec/models/manageiq/providers/embedded_terraform/automation_manager/stack_spec.rb
let(:terraform_runner_url) { "https://1.2.3.4:7000" }
let(:hello_world_retrieve_response) do
require 'json'
JSON.parse(File.read(File.join(__dir__, "../../../../../lib/terraform/runner/data/responses/hello-world-retrieve-success.json")))
Copy link
Member

@agrare agrare Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We might want to consider moving this up to spec/data or something if it is going to be used by more than just the Terraform::Runner specs since this ../../../etc... makes it hard to read where this file lives. Without counting the parent dirs I thought this lived in the "production" lib/terraform directory not spec/lib/terraform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes agree makes it harder to read,
could make it like
"../../../../../../spec/lib/terraform/runner/data/responses/hello-world-retrieve-success.json"

also maybe should move this common test data, to somewhere like <base-dir>/spec/data. then still look like
"../../../../../../spec/data/responses/hello-world-retrieve-success.json"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't know if there more easier way to refer to test data resources without using "../../../../../../"

Copy link
Member

Choose a reason for hiding this comment

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

We can use the ManageIQ::Providers::EmbeddedTerraform::Engine.root Pathname to build a path from the root of the plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

agrare
agrare approved these changes Oct 21, 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 number of items that we can clean up but overall looks good to me

@agrare agrare self-assigned this Oct 21, 2024
@agrare agrare added enhancement New feature or request radjabov/yes? labels Oct 21, 2024
@agrare agrare merged commit 4c66529 into ManageIQ:master Oct 21, 2024
4 checks passed
@agrare agrare mentioned this pull request Oct 21, 2024
Merged
@putmanoj putmanoj deleted the add-method-fetch-terraform-stdout-for-ui branch October 22, 2024 12:24
@Fryguy
Copy link
Member

Fryguy commented Oct 22, 2024

Backported to radjabov in commit ecb9a6c.

commit ecb9a6cdac4edea31897a097fce91181bf18a86e
Author: Adam Grare <[email protected]>
Date:   Mon Oct 21 09:20:36 2024 -0400

    Merge pull request #78 from putmanoj/add-method-fetch-terraform-stdout-for-ui
    
    Add methods to fetch terraform stdout for showing in UI.
    
    (cherry picked from commit 4c66529cb5827112f4c7640f11b705e9258669cd)

Fryguy pushed a commit that referenced this pull request Oct 22, 2024
@agrare @Fryguy
Merge pull request #78 from putmanoj/add-method-fetch-terraform-stdou…
ecb9a6c 
…t-for-ui

Add methods to fetch terraform stdout for showing in UI.

(cherry picked from commit 4c66529)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

@Fryguy Fryguy Fryguy left review comments

Assignees

@agrare agrare

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

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