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 support for Amazon Elastic Transcoder #6965

Merged
merged 6 commits into from
Jun 7, 2016
Merged

Add support for Amazon Elastic Transcoder #6965

merged 6 commits into from
Jun 7, 2016

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jun 1, 2016

No description provided.

phinze
phinze reviewed Jun 1, 2016
View reviewed changes
builtin/providers/aws/resource_aws_elastic_transcoder_pipeline.go
Type: schema.TypeString,
Optional: true,
Computed: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is an output only field? Nix Optional: true to make it read-only.

@jbardin jbardin force-pushed the jbardin/GH-3968 branch 3 times, most recently from a0e57da to 24e2622 Compare June 2, 2016 20:40
@jbardin
Copy link
Member Author

jbardin commented Jun 2, 2016

OK, I think I've pushed the nested schemas too far here, and I can't get this to work reliably. I'll go and flatten out the namespace for now to make this work.

@jbardin jbardin changed the title WIP: Add support for Amazon Elastic Transcoder Add support for Amazon Elastic Transcoder Jun 6, 2016
phinze
phinze reviewed Jun 6, 2016
View reviewed changes
builtin/providers/aws/resource_aws_elastic_transcoder_preset_test.go
name = "aws_elastictranscoder_preset_tf_test_"
audio = {
audio_packing_mode = "SingleTrack"
bit_rate = 128
Copy link
Contributor

Choose a reason for hiding this comment

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

mixed whitespace in a few places in these fixtures

@phinze
Copy link
Contributor

phinze commented Jun 6, 2016

LGTM with the flattening!

@jbardin
Copy link
Member Author

jbardin commented Jun 7, 2016

Ok, working acc tests

$ TF_ACC=1 go test -v -run ElasticTranscoderPipeline
=== RUN   TestAccAWSElasticTranscoderPipeline_basic
--- PASS: TestAccAWSElasticTranscoderPipeline_basic (26.79s)
=== RUN   TestAccAWSElasticTranscoderPipeline_withContentConfig
--- PASS: TestAccAWSElasticTranscoderPipeline_withContentConfig (43.53s)
=== RUN   TestAccAWSElasticTranscoderPipeline_withPermissions
--- PASS: TestAccAWSElasticTranscoderPipeline_withPermissions (27.30s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    97.644s

Added docs too

phinze
phinze reviewed Jun 7, 2016
View reviewed changes
website/source/docs/providers/aws/r/elastic_transcoder_pipeline.html.markdown
name = "aws_elastictranscoder_pipeline_tf_test_"
role = "${aws_iam_role.test_role.arn}"

content_config = {
Copy link
Contributor

Choose a reason for hiding this comment

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

These = are valid but not conventional. Just do content_config { etc.

@phinze
Copy link
Contributor

phinze commented Jun 7, 2016

We are close! A few docs nits for ya and this is ready to go 🚀 🎸

@jbardin
Copy link
Member Author

jbardin commented Jun 7, 2016

Updated docs, and remove extra =s from test configs.

phinze
phinze reviewed Jun 7, 2016
View reviewed changes
website/source/docs/providers/aws/r/elastic_transcoder_preset.html.markdown
* `thumbnails` - (Optional, ForceNew) Thumbnail parameters object (documented below)
* `video` - (Optional, ForceNew) Video parameters object (documented below)
* `video_watermarks` - (Optional, ForceNew) Watermark parameters for the video parameters (documented below)
* `video_codec_options` (Optional, ForceNew) Codec options for the video parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

In the docs our convention here is , Forces new resource

@phinze
Copy link
Contributor

phinze commented Jun 7, 2016

One laaaaaast nit and we're good to go

@jbardin
Copy link
Member Author

jbardin commented Jun 7, 2016

s/ForceNew/Forces new resource/

@phinze
Copy link
Contributor

phinze commented Jun 7, 2016

Yay! LGTM pending travis 🎉

@jbardin jbardin merged commit cd160d3 into master Jun 7, 2016
@jbardin jbardin deleted the jbardin/GH-3968 branch June 7, 2016 21:28
@jbardin jbardin mentioned this pull request Jun 20, 2016
Closed
@ghost
Copy link

ghost commented Apr 25, 2020

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement provider/aws
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jbardin @phinze @catsby @radeksimko