Inventory Enhancements (Resource and Data Source) #62
Conversation
lranjbar
force-pushed
the
update-inventory-data-source
branch
2 times, most recently
from
4285a4b to
2ac0f8e
Compare
|
Overall this is looking good, Also, some of the functions seem like their arguments could be replaced with interfaces, only if, terraform dosent try to use reflection when calling these functions. 😄 |
lranjbar
force-pushed
the
update-inventory-data-source
branch
2 times, most recently
from
c157b16 to
87673a9
Compare
lranjbar
changed the title
lranjbar
force-pushed
the
update-inventory-data-source
branch
2 times, most recently
from
bdfc4a2 to
1f15818
Compare
* Cleanup string parsing * Add tests fror ValidateLookupParameters * Add more test cases
lranjbar
force-pushed
the
update-inventory-data-source
branch
from
1f15818 to
a971941
Compare
lranjbar
force-pushed
the
update-inventory-data-source
branch
from
a971941 to
212f852
Compare
|
Acceptance tests output |
There was a problem hiding this comment.
Thanks for running the tests and posting the output. Excited to see these enhancements coming 😄. Overall I think this looks really good!
After my last review we had some good discussion around some docs suggestions (required->optional, reference fornamed_url). I thought we had a good direction from the discussion with some small changes. I see the comments are now resolved but there were no code changes. I don't want this to get lost, is there a plan to address it?
My other feedback was around the acceptance tests. I see one has been updated to verify the new attributes are present, so that's great 👍. However, one that was active is now being skipped (which seems like a regression). Most importantly, the use case we're aiming to support (being able to use a data.aap_inventory to look up an inventory by name) is not covered by an acceptance test at all. Is that a test we can write?
Co-authored-by: Dan Leehr <[email protected]>
|
With the Job template set to prompt on launch we are still passing the test case for inventoryid. 🎉 The output of JobResource tests below: ==> Running acceptance tests... |
Acceptance tests were updated, and now docs updated so that should cover everything that has been discussed. |
OK I see that an acceptance test for the inventory resource was updated to check for the new attributes, but I do not see any new acceptance tests around the new functionality described in the story:
It's good that we're testing these changes (no regressions 🎉) but we need new acceptance tests that follow the user story - show that we can look up an inventory by
The Jira has a pretty basic terraform config we should be able to use in an acceptance test to confirm. Maybe we need organization name too, but generally we'd use something like this, right? data "aap_inventory" "my_inventory" {
name = "my_inventory" # no id
}(P.S. can we check if we're really addressing #52 with these changes? I'm not sure this PR closes that issue. 👀 ) |
Part of AAP-44426
User Story
As a user of Terraform and AAP, I want the terraform-provider-aap to implement data sources for the Job Template and Inventory objects in AAP so that I can look up existing objects when creating AAP jobs from Terraform.