Skip to content

Conversation

@stevethomas
Copy link
Member

@stevethomas stevethomas commented Sep 30, 2025

Hey, I made a thing! 🥳

Great! Now please answer the following questions to help out your assigned reviewer:

What problems are you solving?

  • drops AWS Elastic Transcoder and switches to MediaConvert given that the former will be EOL in Nov 2025
  • Add AWSElementalMediaConvertFullAccess to EC2 role
  • Create a service role for MediaConvert and inject to .env to allow SDK to create jobs
  • Rename EC2 role steps for improved clarity
  • expand env Ensure step to check APP_VERSION and ASSET_URL
  • throw an exception if YOLO_$ENV_AWS_PROFILE is default / null for safety

Is there anything the reviewer needs to know to deploy this?

  • sync:iam will need to run in each environment. This will set the necessary service role for MediaConvert to assume when running, as well as allow the EC2 role to manage MediaConvert.
  • Note that the Elastic Transcoder permission will be removed when this runs, so the deployment should very closely follow (OR change to a 2-step deployment and remove the permission later)

@stevethomas stevethomas changed the title wip MediaConvert Sep 30, 2025
@stevethomas stevethomas marked this pull request as ready for review October 2, 2025 23:37
@stevethomas stevethomas merged commit 2f63c35 into main Oct 2, 2025
4 checks passed
@stevethomas stevethomas deleted the steve/lp-1811-move-transcoding-to-a-job branch October 2, 2025 23:39
Comment on lines +36 to +37
if (empty($dotenv['APP_VERSION'])) {
$this->throwException($dotenv, 'APP_VERSION');
Copy link

Choose a reason for hiding this comment

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

Potential bug: Direct access to keys in the $dotenv array without an existence check can cause 'Undefined array key' errors if environment variables are missing.
  • Description: The EnsureEnvIsConfiguredCorrectlyStep validates environment variables. However, when a variable like ASSET_URL or AWS_MEDIACONVERT_ROLE_ID is missing, the code accesses the $dotenv array without checking if the key exists. This occurs both in direct comparisons and within the throwException method when constructing the error message ({$dotenv[$key]}). This results in a PHP 'Undefined array key' warning. In environments with strict error reporting, this warning will be treated as an error, causing the deployment to crash instead of gracefully reporting the missing configuration.

  • Suggested fix: Before accessing keys from the $dotenv array, verify their existence using isset() or the null coalescing operator (??). For example, in the throwException method, change {$dotenv[$key]} to {$dotenv[$key] ?? 'not set'} to prevent errors when constructing the exception message for a missing key.
    severity: 0.7, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants