- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 354
 
Fixes / improvements for AutoScaling #334
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
Conversation
      
          
      
      
            lanej
  
      
      
      commented
        Dec 15, 2016 
      
    
  
- Restore passing live tests
 - Fix setting tags on #create_auto_scaling_group
 
These tests reference objects that do not exist and do not succeed in a live environment.
During a live test run there are quite a few suspended processes.
```
suspend processes
	processes suspended - returns []
		expected => []
		returned => [{"ProcessName"=>"RemoveFromLoadBalancerLowPriority", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"Launch", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"HealthCheck", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"AddToLoadBalancer", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"AlarmNotification", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"ScheduledActions", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"AZRebalance", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"Terminate", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}, {"ProcessName"=>"ReplaceUnhealthy", "SuspensionReason"=>"User suspended at 2016-12-15T19:29:32Z"}]
```
    Converts
```
AWS::AutoScaling | tag requests (aws, auto_scaling)
	1 validation error detected: Value '{"Key"=>"Name", "PropagateAtLaunch"=>true, "ResourceId"=>"fog-test-1481831027", "ResourceType"=>"auto-scaling-group", "Value"=>"fog-test-1481831027"}' at 'tags.1.member.key' failed to satisfy constraint: Member must have length less than or equal to 128 (Fog::AWS::AutoScaling::ValidationError)
```
to
```
AWS::AutoScaling | tag requests (aws, auto_scaling)
  1 validation error detected: Value '{"Key"=>"Name", "PropagateAtLaunch"=>true, "ResourceId"=>"fog-test-1481831149", "ResourceType"=>"auto-scaling-group", "Value"=>"fog-test-1481831149"}' at 'tags.1.member.key' failed to satisfy constraint: Member must have length less than or equal to 128 (Fog::AWS::AutoScaling::ValidationError)
```
    | 
           Thanks!  | 
    
| 
           The changes in 2937ab1 seem to have broken autoscaling group creation for https://github.com/scalefactory/build-cloud/ (we hadn't spotted the symptoms until just now)  | 
    
| options["Tags.member.#{i+1}.Key"] = key.to_s # turns symbol into string | ||
| options["Tags.member.#{i+1}.Value"] = value | ||
| end | ||
| options.merge!(AWS.indexed_param("Tags.member.%d", [*tags])) | 
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 this change is incorrect, and leads to a tag of Name=foo to be passed to the API as Tags.member.1=[Name,foo] instead of Tags.member.1.key=Name&Tags.member.1.value=foo. As far as I can tell the AWS.indexed_param method is used elsewhere in the project only for lists of values, not hashes. I think this change needs to be reverted, but I haven't yet been able to test this fully.
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.
Leaving this for Google to find: the error message caused by this problem is Fog::AWS::AutoScaling::Error: MalformedInput => Unexpected complex element termination")
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.
@lanej I'm a bit removed from all this these days, maybe you have a bit nearer insight into to comment?