-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Set platform override in imagebuilder container recipe. #24712
Set platform override in imagebuilder container recipe. #24712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome @chrisgilmerproj 👋
It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.
Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.
Thanks again, and welcome to the community! 😃
@justinretzolk - I would really like to get some help finalizing this PR. It's a feature I need for production and if someone can give me some guidance I'm willing to do the rest of the work here. Can you help me find someone to partner with? |
Hey @chrisgilmerproj 👋 First, thank you for taking the time to raise this PR! I'll reach out to some of our frequent contributors and see if I can get someone to take another look as well, but I've got a couple of resources that'll help you out. One document that may help some is the contribution checklist for enhancements. Looking at that, you've got most of the things in order already, but you'll want to add a documentation update to document the new argument you've added. You'll also want to add a changelog item. The process to do so is described here. There's also the common review items checklist that may be of use. Finally, I'd be remiss if I didn't mention our prioritization guide to give you an idea of the lifecycle of the pull request itself. |
@justinretzolk - Thanks for the help. I have added a changelog and docs as requested. The thing I need help with is the acceptance test. To test this it requires an AWS ECR image is referenced. If the AWS ECR image does not exist then I think the test will fail. This code isn't making a cross-service call itself so the error is coming from AWS itself. Do you think someone has experience with this kind of problem before? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisgilmerproj - just FYI I'm not a maintainer but a contributor just like yourself. I happen to stick my nose in from time to time though to help with PR reviews and trying to guide folks where I can.
If you need a hand with the test case suggestion I make below, then I'm happy to, I just didn't want to hand-hold too much!
Thanks @mattburgess ! Your suggestions are really helpful. I've committed the ones that are straight forward. I'll go try to update the tests with your suggestion and see how far I get. |
@mattburgess - I ran a test and got an interesting result. I'm not sure how to import the
|
That was a new one on me, but https://www.terraform.io/plugin/sdkv2/resources/import#importstateverifyignore-1 hints at the answer (and it's not to ignore the problem 😄) - I think you just need to update |
It appears there's an issue with the windows version of this. The error I keep getting tells me to set the |
For the error I'm seeing I tried to debug by making this change: diff --git a/internal/service/imagebuilder/container_recipe.go b/internal/service/imagebuilder/container_recipe.go
index 5f2a62e5ed..ff193cb225 100644
--- a/internal/service/imagebuilder/container_recipe.go
+++ b/internal/service/imagebuilder/container_recipe.go
@@ -352,6 +352,7 @@ func resourceContainerRecipeCreate(d *schema.ResourceData, meta interface{}) err
if v, ok := d.GetOk("working_directory"); ok {
input.WorkingDirectory = aws.String(v.(string))
}
+ fmt.Printf("%v\n", input)
output, err := conn.CreateContainerRecipe(input)
The output looked like this:
For some reason my |
Aha, fixed my problem. But now I'm stuck trying to recapture the data that isn't available:
|
I've got the tests working now! It turns out that I can ignore the This makes me think I should also do that for |
Yes, I think you might actually be right there. I see now that ContainerRecipe doesn't contain either of the |
@mattburgess - I think I've done it. I have a good set of tests in both the positive and negative. I also cleaned up the platform import that I had from before. I'd love to know if you were able to run my tests and if everything else looks good to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just some minor nits; will try and get the tests going shortly!
Just to be sure I didn't mess up the other tests:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nearly there 😄 - tests are passing for me with one exception noted below. I can't test the Windows variant of these tests though as I can't pull and push Windows images without a Windows machine.
I've done an update and now the tests look like:
|
Co-authored-by: Matthew Burgess <549318+mattburgess@users.noreply.github.com>
Co-authored-by: Matthew Burgess <549318+mattburgess@users.noreply.github.com>
Co-authored-by: Matthew Burgess <549318+mattburgess@users.noreply.github.com>
I just rebased this on |
Checking in on this. Is it possible to get someone to review it? |
Another check in. Hoping to see some traction on this. |
Hey @chrisgilmerproj 👋 Thank you for checking in on this. Unfortunately, I'm not able to provide an estimate on when this will be merged/reviewed due to the potential of shifting priorities (we prioritize work by count of ":+1:" reactions, as well as a few other things). For more information on how we prioritize, check out out prioritization guide. As a note, you won't need to worry about rebasing to keep the branch up to date; we won't let that affect the prioritization, and if any rebasing is needed at review time, the maintainers will gladly take care of that. |
Thanks @justinretzolk - I'll work on finding more folks to add "👍" reactions so we can get this prioritized. |
@LightningStairs thanks for the approval! Do I need to resolve conflicts on this branch for you? |
is there any updates on this PR being merged in? |
@sf-mporte - I have not heard any news on getting this merged. I wish I had more info for you. |
@chrisgilmerproj I included this change in my recent PR (#30398) as I needed it to write some acceptance tests. My PR has been merged and the changes are set to be included in v4.62.0! |
@samwyma thanks for helping me get these changes into the code base! I do appreciate it as I would like to have these features available and I've been waiting a long time. I would have liked it more if my contributions had also made it into the repo with my username attached. I was looking forward to showing my team and my friends that I had actually contributed to this provider but now the |
So glad to hear! thank you! |
Thank you for your work on this! i hope you get the credit you deserve |
I appreciate it. I noticed the PR #30398 didn't include all of the test cases that I provided in this MR. I'll update the tests and perhaps you can help me get them merged in. |
@sf-mporte @samwyma @LightningStairs @justinretzolk - I have updated this MR to include the most recent changes from Is it possible to get this reviewed and merged as well to go out in the v4.62.0 release? or the following release? Thank you. Test results below:
|
@gdavison This is an extension to the PR you merged for me yesterday which makes the |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Community Note
Closes #24711
Output from acceptance testing:
Prerequisites: Export these environment variables:
And all the platform override tests: