-
Notifications
You must be signed in to change notification settings - Fork 31
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
add internal_name to service #202
Conversation
@@ -2,6 +2,7 @@ class CreateServices < ActiveRecord::Migration | |||
def change | |||
create_table :services do |t| | |||
t.string :name | |||
t.string :internal_name |
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.
This is our first schema change since being out in the wild -- I think we might need to do this as a dedicated migration.
5c83ef4
to
793edc5
Compare
@@ -0,0 +1,5 @@ | |||
class AddInternalNameToService < ActiveRecord::Migration | |||
def change | |||
add_column :services, :internal_name, :string |
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.
I'm not well-versed in the migration stuff, but I'm thinking that we probably also need to do some sort of data manipulation as part of this. If the user already has services in the their DB we'll want to update all those records and copy the name
field to the internal_name
field. Not sure if that logic is appropriate to put in the migration file or if it goes someplace else. Maybe @alexwelch can weigh-in.
793edc5
to
3faff8b
Compare
@@ -29,6 +29,7 @@ class Service < ActiveRecord::Base | |||
serialize :volumes, Array | |||
|
|||
before_save :resolve_name_conflicts | |||
before_create :set_internal_name |
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.
I think the significance of this line is definitely worth exercising in your tests. Specifically the fact that this internal_name is not updated on subsequent changes (updates)... Hit me up if you want me to ellaborate
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.
Updated to add another testing case.
3faff8b
to
98b3dd0
Compare
👍 |
1 similar comment
👍 |
98b3dd0
to
10c7bc6
Compare
10c7bc6
to
ae651c9
Compare
We want users to be able to update service.name, but that value is used within the unit file. This introduces a new field on the service called 'internal_name' which is immutable and will be used in places where a string is used a unique identifier.
[finishes #70098068]