Skip to content

Conversation

@grahamc
Copy link
Contributor

@grahamc grahamc commented Nov 23, 2020

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

* **New Resource:** aws_ebs_snapshot_import  (#16373)

Output from acceptance testing:

$ time make fmt testacc TESTARGS='-run=TestAccAWSEBSSnapshotImport'
==> Fixing source code with gofmt...
gofmt -s -w ./aws ./awsproviderlint/helper ./awsproviderlint/main.go ./awsproviderlint/passes
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEBSSnapshotImport -timeout 120m
=== RUN   TestAccAWSEBSSnapshotImport_basic
=== PAUSE TestAccAWSEBSSnapshotImport_basic
=== CONT  TestAccAWSEBSSnapshotImport_basic
--- PASS: TestAccAWSEBSSnapshotImport_basic (625.66s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	25.694s

real	12m33.506s
user	2m43.535s
sys	2m7.373s


I don't feel incredibly comfortable that the code I wrote is good or totally right, but I gave it my best. I'm not much of a Go programmer, and started with the ebs_snapshot resource. Note I didn't add support for importing, because it seems the data around the import parameters are "Disappeared" once the import task completes.

My use case here is combining aws_ebs_snapshot_import with aws_ami and aws_ami_copy to upload, register, and distribute AMIs to a variety of regions. I use Terraform to support the pruning of images over time.

@grahamc grahamc requested a review from a team as a code owner November 23, 2020 14:24
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 23, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Nov 23, 2020
Copy link
Contributor Author

@grahamc grahamc Nov 23, 2020

Choose a reason for hiding this comment

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

This is a bit funny probably, but to test importing a disk image we need a disk image to import. The image on disk, base64'd, is about 3400 bytes long. Compressing, then base64'ing is much shorter, with the trade-off that we need to decompress and unbase64 on the fly for the test.

@grahamc grahamc force-pushed the f-aws_ebs_snapshot_import branch from dc9796a to cd6cd2a Compare November 23, 2020 14:29
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome @grahamc 👋

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! 😃

@grahamc grahamc force-pushed the f-aws_ebs_snapshot_import branch 3 times, most recently from 8d5c8fe to 64b1c8f Compare November 23, 2020 15:06
@grahamc grahamc force-pushed the f-aws_ebs_snapshot_import branch 3 times, most recently from e8224bb to beca575 Compare November 23, 2020 15:42
@grahamc grahamc changed the title aws_ebs_snapshot_import: init resource aws_ebs_snapshot_import: init resource (the red X is from a panic() I don't know how to avoid) Nov 23, 2020
@grahamc grahamc changed the title aws_ebs_snapshot_import: init resource (the red X is from a panic() I don't know how to avoid) aws_ebs_snapshot_import: init resource (the red X is from a panic() in a test which I don't know how to avoid) Nov 24, 2020
@grahamc grahamc force-pushed the f-aws_ebs_snapshot_import branch from beca575 to 89e63ef Compare December 7, 2020 17:26
@ghost ghost added the documentation Introduces or discusses updates to documentation. label Dec 7, 2020
@grahamc grahamc changed the title aws_ebs_snapshot_import: init resource (the red X is from a panic() in a test which I don't know how to avoid) aws_ebs_snapshot_import: init resource Dec 7, 2020
@grahamc grahamc force-pushed the f-aws_ebs_snapshot_import branch from 89e63ef to ce70760 Compare January 11, 2021 22:16
@grahamc
Copy link
Contributor Author

grahamc commented Jan 11, 2021

Just rebased this on top of master, and with it I added tags to the test case after discovering the previous revision of this PR didn't correctly tag the snapshot.

Base automatically changed from master to main January 23, 2021 00:59
@grahamc grahamc force-pushed the f-aws_ebs_snapshot_import branch from ce70760 to 3e5589a Compare March 2, 2021 23:54
@grahamc
Copy link
Contributor Author

grahamc commented Mar 2, 2021

Rebased and re-ran the tests.

@m4h3
Copy link

m4h3 commented Apr 23, 2021

can this get some attention ?

@grahamc

This comment has been minimized.

@grahamc grahamc changed the title aws_ebs_snapshot_import: init resource aws_ebs_snapshot_import: init resource Apr 23, 2021
@grahamc
Copy link
Contributor Author

grahamc commented Jun 16, 2021

I heard from a Hashicorp person on Twitter that this PR might get looked at soon.

Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

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

Not a maintainer but want to help move this along

also add disappears-acceptance-tests

return nil
}

func importAwsEbsSnapshotWaiter(conn *ec2.EC2, importTaskID string) (*ec2.SnapshotTaskDetail, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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


func TestAccAWSEBSSnapshotImport_basic(t *testing.T) {
var v ec2.Snapshot
rName := fmt.Sprintf("tf-acc-ebs-snapshot-import-basic-%s", acctest.RandString(7))
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets use the more common rName := acctest.RandomWithPrefix("tf-acc-test")

Comment on lines 22 to 24
Read: resourceAwsEbsSnapshotImportRead,
Update: resourceAwsEbsSnapshotImportUpdate,
Delete: resourceAwsEbsSnapshotImportDelete,
Copy link
Collaborator

Choose a reason for hiding this comment

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

i wonder if these are needed and can use existing snapshot function similar to AMI resources. see resource_aws_ami_copy for instance

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a look a couple days ago and didn't think that it would be worth it to pursue further. There were enough differences that (IIRC) it wouldn't be drop-in.

@DrFaust92
Copy link
Collaborator

also add changelog file

@grahamc grahamc force-pushed the f-aws_ebs_snapshot_import branch from 1522f8c to 57667a4 Compare June 28, 2021 20:02
@grahamc
Copy link
Contributor Author

grahamc commented Jun 28, 2021

Thanks, @DrFaust92! I've had @cole-h help me out and address your feedback. I also rebased on main and it looks like everything is still working from here. CI is also green, but requires approval to run a few more checks.


func testAccAwsEbsSnapshotImportConfigBasic(rName string, t *testing.T) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "images" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets extract the common part between configs for reuse

})
}

func TestAccAWSEBSSnapshotImport_tags(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type: schema.TypeString,
Computed: true,
},
"tags": tagsSchema(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DrFaust92 DrFaust92 added new-resource Introduces a new resource. and removed needs-triage Waiting for first response or review from a maintainer. labels Jun 29, 2021
@cole-h
Copy link
Contributor

cole-h commented Jun 29, 2021

Once again, thanks for your time and review, @DrFaust92! I've fixed your suggestions in a few new commits, and testacc still passes.

@cole-h cole-h force-pushed the f-aws_ebs_snapshot_import branch from 577efd5 to cb8ec76 Compare July 15, 2021 16:34
@cole-h
Copy link
Contributor

cole-h commented Jul 15, 2021

Rebased to resolve a conflict.

@bill-rich bill-rich force-pushed the f-aws_ebs_snapshot_import branch from cd8233b to 4930305 Compare July 27, 2021 16:55
@grahamc
Copy link
Contributor Author

grahamc commented Jul 27, 2021

❤️

@bill-rich bill-rich merged commit cd9ff2a into hashicorp:main Jul 27, 2021
@grahamc grahamc deleted the f-aws_ebs_snapshot_import branch July 27, 2021 17:31
@github-actions github-actions bot added this to the v3.52.0 milestone Jul 27, 2021
@grahamc
Copy link
Contributor Author

grahamc commented Jul 27, 2021

🥳 wow, thank you so much Bill!

@github-actions
Copy link
Contributor

This functionality has been released in v3.52.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link
Contributor

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/ec2 Issues and PRs that pertain to the ec2 service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants