Allow AMI and Security Group selection (in combination with PR on Tango) #2286
Conversation
…ing in case they don't select options).
📝 WalkthroughWalkthroughThe pull request introduces enhanced autograder configuration with EC2-specific options. A new JavaScript file adds a callback for toggling access key fields based on a checkbox. Controller actions are updated to initialize and permit new attributes, and additional Tango information is extracted in jobs actions. The helper method updating job properties now incorporates EC2 configurations. New view partials are created for basic and EC2 settings and integrated into a tabbed interface. Feature flags for EC2 SSH are added to environment configurations, and migrations along with schema updates introduce new columns, adjust data types, and add foreign keys. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CB as Checkbox (#autograder_use_access_key)
participant JS as Autograder Callback
participant AK as Input Field (#autograder_access_key)
participant AID as Input Field (#autograder_access_key_id)
Note over JS: Callback attached on document ready
U->>CB: Toggle checkbox
CB->>+JS: Trigger change event
alt Checkbox is checked
JS->>AK: Enable field
JS->>AID: Enable field
else Checkbox is unchecked
JS->>AK: Disable field and clear value
JS->>AID: Disable field and clear value
end
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
🧰 Additional context used🪛 RuboCop (1.73)db/schema.rb[convention] 14-15: Extra empty line detected at block body beginning. (Layout/EmptyLinesAroundBlockBody) 🪛 GitHub Actions: Ruby on Rails CIdb/schema.rb[error] 444-444: ActiveRecord::MismatchedForeignKey: Column 🔇 Additional comments (16)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (4)
db/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rb (1)
1-7: Consider security implications of storing AWS keys in the database.The migration adds fields for AWS credentials directly in the database. While the implementation works, consider these security recommendations:
- Ensure access keys are encrypted at rest in the database
- Consider using AWS-provided credential management systems instead of storing raw keys
- Add validation to prevent empty strings from being used as valid credentials
This approach might be suitable for your current needs, but document the security considerations for future maintainers.
app/views/autograders/_ec2_settings.html.erb (3)
13-16: Inconsistent form field styling.Unlike other form fields that use display_name, the AMI label is placed outside the form construct, creating an inconsistent UI pattern.
Maintain consistent styling by using display_name for the AMI selector:
-AMI -<%= f.select :ami, +<%= f.select :ami, display_name: "AMI", options_for_select(@tagged_amis.map { |ami| ["Name: #{ami['name']}, ID: #{ami['id']}", ami['id']] }, f.object.ami), { include_blank: "Use Tango Default" } %>
18-21: Inconsistent form field styling.Similar to the AMI field, the Security Group label is placed outside the form construct.
Maintain consistent styling by using display_name for the Security Group selector:
-Security Group -<%= f.select :security_group, +<%= f.select :security_group, display_name: "Security Group", options_for_select(@security_groups.map { |sg| ["Name: #{sg['name']}, ID: #{sg['id']}", sg['name']] }, f.object.security_group), { include_blank: "Use Tango Default" } %>
24-26: Fix typo in confirmation message.There's a spelling error in the confirmation message for deleting the autograder.
- data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %> + data: { confirm: "Are you sure you want to delete the Autograder for this assessment?" } %>
🛑 Comments failed to post (2)
app/views/autograders/_ec2_settings.html.erb (1)
9-10: 🛠️ Refactor suggestion
Enhance security for access key fields.
The access key and access key ID are sensitive AWS credentials displayed as plain text fields. This poses a security risk as these credentials could be exposed to unauthorized viewers.
Change the input type to password for these sensitive fields:
-<%= f.text_field :access_key, display_name: "Access Key" %> -<%= f.text_field :access_key_id, display_name: "Access Key ID" %> +<%= f.password_field :access_key, display_name: "Access Key" %> +<%= f.password_field :access_key_id, display_name: "Access Key ID" %>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<%= f.password_field :access_key, display_name: "Access Key" %> <%= f.password_field :access_key_id, display_name: "Access Key ID" %>db/schema.rb (1)
18-19: 💡 Verification agent
🧩 Analysis chain
Critical issue: Mismatched column types in Active Storage tables.
The pipeline is failing because column types for foreign keys in Active Storage tables have been changed from
biginttointeger, causing a mismatch with their referenced columns.This needs to be fixed as Rails expects these columns to be
bigint. You should create a migration to revert these column types back tobigint:class FixActiveStorageColumnTypes < ActiveRecord::Migration[6.1] def change change_column :active_storage_attachments, :record_id, :bigint change_column :active_storage_attachments, :blob_id, :bigint change_column :active_storage_blobs, :byte_size, :bigint change_column :active_storage_variant_records, :blob_id, :bigint end endAlso applies to: 38-39
🏁 Script executed:
#!/bin/bash # Check the column types in migrations affecting Active Storage tables grep -r "active_storage" --include="*.rb" db/migrate/ | grep -E "change_column|add_column"Length of output: 402
Critical Issue: Active Storage Foreign Key Column Type Mismatch
The schema in
db/schema.rb(lines 18–19 and 38–39) defines therecord_idandblob_idcolumns asinteger, yet Rails expects these to bebigintfor proper foreign key references. The migration check did not reveal any changes addressing this mismatch—only an update for theservice_namecolumn was found.Please create a migration to change these columns back to
bigint. For example:class FixActiveStorageColumnTypes < ActiveRecord::Migration[6.1] def change change_column :active_storage_attachments, :record_id, :bigint change_column :active_storage_attachments, :blob_id, :bigint change_column :active_storage_blobs, :byte_size, :bigint change_column :active_storage_variant_records, :blob_id, :bigint end end
Description
Allow user to choose an AMI and Security Group EC2 Settings. Options are pooled from Tango.
Motivation and Context
#2272
How Has This Been Tested?
Ensured AMI and Security Group lists were properly pooled from Tango.
Change autograder settings and grade various submissions, ensure the AMI and Security Group changes carry through.
Types of changes