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.

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

Add terraform state inventory source #2209

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

hakbailey
Copy link
Collaborator

Note: this is a resubmission of PR 1868 from a branch instead of a fork so checks can run. See previous review comments there.

Summary:

A new Terraform state inventory source was added to AWX in PR 14840. This PR updates the UI to include the new source type. It also adds Execution Environment selection to the inventory source add/edit forms, as an EE selection is required for the Terraform source -- users must provide and select an EE containing a Terraform binary, as we cannot ship one with the Terraform binary for licensing reasons.

In addition, this PR fixes a couple of related bugs: when trying to edit an inventory source that is not sourced from a project, the API returns a 400: Bad request error with detail "Cannot set source_path if not SCM type." due to a default value in the form for the source_path field. And the cypress deleteAwxExecutionEnvironment command had the wrong endpoint so this fixes that as well.

@github-actions github-actions bot added AWX Label to indicate changes relevant to AWX E2E End-to-end testing labels Apr 30, 2024
Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

Looks really good. thanks for resubmitting this pr. Just a few small things...and 1 lingering question for people about the documentation.

@@ -15,4 +15,6 @@ export const ansibleDocUrls: { [key: string]: string } = {
'https://docs.ansible.com/ansible/latest/collections/community/vmware/vmware_vm_inventory_inventory.html',
constructed:
'https://docs.ansible.com/ansible/latest/collections/ansible/builtin/constructed_inventory.html',
terraform:
'https://github.com/ansible-collections/cloud.terraform/blob/main/plugins/inventory/terraform_state.py',
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure we want to link out to a github repo as official documentation. I'm seeking input from people who would know.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, did some digging. Docs for this are pending publication. The issue can be tracked here https://issues.redhat.com/browse/ACA-1187. Might be worth while reaching out on that issue to see if they know the url that would link to terraform docs yet. Sometimes they do know ahead of time where they will go. For what its worth I suggested to Lynn Maynard that terraform docs should follow suit with the list of links to docs here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AlexSCorey that issue covers docs for the Terraform State inventory source in AAP, whereas here I think we want to link to the docs for the inventory plugin itself (which is also linked from those docs) so folks can understand what the plugin options are. We haven't done a cloud.terraform release yet with the new plugin so we can't link to published docs...we can either wait until that release happens and link to the plugin docs on Automation Hub, or we can link to the repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AlexSCorey I updated the docs link and rebased onto current main to fix a merge conflict in the cypress test file (part of the credential test had been changed since I last worked on this). It looks like there are two failing tests but I don't think they are related to this PR. Let me know if any other changes are needed, thanks!

Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

@hakbailey and I met offline and addressed all of my concerns. This will merge after the cloud documentation merges.

@gravesm
Copy link
Member

gravesm commented May 23, 2024

@hakbailey hakbailey force-pushed the add-terraform-state-inventory-source branch from 4009a14 to b6d1a4f Compare May 23, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AWX Label to indicate changes relevant to AWX E2E End-to-end testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants