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

fix(provider/amazon) Enable & fix existing "Create LB" stage #4184

Merged
merged 6 commits into from
Oct 3, 2017
Merged

fix(provider/amazon) Enable & fix existing "Create LB" stage #4184

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ad7d3ae
Enable & fix existing "Create LB" stage
Sep 26, 2017
87be22a
Merge branch 'master' into ELB_stage
dotdotdotpaul Sep 29, 2017
ccbc32e
fix: take out leftover console.log debug message
Sep 29, 2017
34abccc
fix(providers/amazon): Fix editing existing LB
Sep 29, 2017
5e88807
fix(providers/amazon): Respond to comments.
Oct 2, 2017
7225922
Merge branch 'master' into ELB_stage
anotherchrisberry Oct 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions app/scripts/modules/amazon/src/loadBalancer/configure/choice/awsLoadBalancerChoice.modal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,32 +11,48 @@ class AwsLoadBalancerChoiceCtrl implements IController {

constructor(private $uibModal: any,
private $uibModalInstance: IModalInstanceService,
private application: Application) {
private application: Application,
private loadBalancer: any,
private isNew: boolean,
private forPipelineConfig: boolean) {
'ngInject';
}

public $onInit(): void {
this.choices = LoadBalancerTypes;
this.choice = this.choices[0];
if (this.loadBalancer) {
// If we're editing an existing LB, preset the choice based on config,
const needed_type = this.loadBalancer.loadBalancerType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor style nit: we typically use camelCase for variable names.

this.choice = LoadBalancerTypes.filter(function(el) {
return el.type === needed_type;
})[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Whole thing can be simplified to:

this.choice = LoadBalancerTypes.find(t => t.type === this.loadBalancer.loadBalancerType);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this will solve the camelcase problem, too, so I'll see if it works. I was having trouble getting the scope to work (or at least, that's what I thought was the problem).

}
}

public onChoiceSelection(choice: IAwsLoadBalancerConfig): void {
this.choice = choice;
}

public choose(): void {
this.$uibModalInstance.dismiss();
this.$uibModal.open({
templateUrl: this.choice.createTemplateUrl,
controller: `${this.choice.controller} as ctrl`,
size: 'lg',
resolve: {
application: () => this.application,
loadBalancer: (): null => null,
isNew: () => true,
forPipelineConfig: () => false,
}
});
// NOTE: Can't just dismiss here, pipelineconfig is expecting the
// result of the config to be passed back as a promise from
// the initial modal, which we're closing here...
this.$uibModalInstance.close(
this.$uibModal.open({
templateUrl: this.choice.createTemplateUrl,
controller: `${this.choice.controller} as ctrl`,
size: 'lg',
resolve: {
application: () => this.application,
loadBalancer: () => this.loadBalancer,
isNew: () => this.isNew || (this.loadBalancer == null),
forPipelineConfig: () => this.forPipelineConfig,
}
}).result.then(function(newLoadBalancer: any) {
return newLoadBalancer;
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this last then block is necessary - looks like you should be able to just do:

    this.$uibModalInstance.close(
      this.$uibModal.open({
        templateUrl: this.choice.createTemplateUrl,
        controller: `${this.choice.controller} as ctrl`,
        size: 'lg',
        resolve: {
          application: () => this.application,
          loadBalancer: () => this.loadBalancer,
          isNew: () => this.isNew || (this.loadBalancer == null),
          forPipelineConfig: () => this.forPipelineConfig,
        }
      }).result);

(I could be reading this wrong, too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went through several permutations (this is my first attempt at doing Angular) so you may be right, I'll try it and see. Definitely cleaner if I can do without.

);
}
}

Expand Down
2 changes: 2 additions & 0 deletions ...s/modules/amazon/src/loadBalancer/configure/common/createAmazonLoadBalancer.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export abstract class CreateAmazonLoadBalancerCtrl {
private regions: IRegion[];
public subnets: ISubnetOption[];
private taskMonitor: TaskMonitor;
public enforceUniqueName: boolean;

constructor(protected $scope: IScope,
protected $uibModalInstance: IModalInstanceService,
Expand All @@ -89,6 +90,7 @@ export abstract class CreateAmazonLoadBalancerCtrl {
// if this controller is used in the context of "Create Load Balancer" stage,
// then forPipelineConfig flag will be true. In that case, the Load Balancer
// modal dialog will just return the Load Balancer object.
this.enforceUniqueName = !this.forPipelineConfig;

this.viewState = {
accountsLoaded: false,
Expand Down
13 changes: 11 additions & 2 deletions app/scripts/modules/amazon/src/loadBalancer/configure/common/createLoadBalancerLocation.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ <h3 us-spinner="{radius:30, width:8, length: 16}"></h3>
</div>
<div class="modal-body" ng-if="ctrl.viewState.accountsLoaded">
<div class="form-group">
<div class="col-md-12 well" ng-class="{'alert-danger': form.loadBalancerName.$error.validateUnique, 'alert-info': !form.loadBalancerName.$error.validateUnique}">
<div class="col-md-12 well" ng-class="{'alert-danger': form.loadBalancerName.$error.validateUnique && ctrl.enforceUniqueName, 'alert-info': !form.loadBalancerName.$error.validateUnique}">
<strong>Your load balancer will be named:</strong>
<span>{{ctrl.getName()}}</span>
<!-- Angular does not seem to run length validation on hidden inputs, hence the text + display:none -->
Expand All @@ -15,7 +15,16 @@ <h3 us-spinner="{radius:30, width:8, length: 16}"></h3>
ng-model="ctrl.loadBalancerCommand.name"
validate-unique="ctrl.existingLoadBalancerNames"
validate-ignore-case="true"
name="loadBalancerName"/>
name="loadBalancerName"
ng-if="ctrl.enforceUniqueName" />
<input type="text"
style="display: none"
ng-maxlength="32"
class="form-control input-sm no-spel"
ng-model="ctrl.loadBalancerCommand.name"
validate-ignore-case="true"
name="loadBalancerName"
ng-if="!ctrl.enforceUniqueName" />
<validation-error ng-if="form.loadBalancerName.$error.validateUnique" message="There is already a load balancer in {{ctrl.loadBalancerCommand.credentials}}:{{ctrl.loadBalancerCommand.region}} with that name."></validation-error>
</div>
</div>
Expand Down
24 changes: 12 additions & 12 deletions app/scripts/modules/amazon/src/loadBalancer/loadBalancer.transformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,21 +134,21 @@ export class AwsLoadBalancerTransformer {
isInternal: loadBalancer.isInternal,
region: loadBalancer.region,
cloudProvider: loadBalancer.cloudProvider,
credentials: loadBalancer.account,
listeners: [],
credentials: loadBalancer.credentials,
listeners: loadBalancer.listeners,
loadBalancerType: 'classic',
name: loadBalancer.name,
regionZones: loadBalancer.availabilityZones,
securityGroups: [],
vpcId: undefined,
securityGroups: loadBalancer.securityGroups,
vpcId: loadBalancer.vpcId,
healthCheck: undefined,
healthTimeout: undefined,
healthInterval: undefined,
healthyThreshold: undefined,
unhealthyThreshold: undefined,
healthCheckProtocol: undefined,
healthCheckPort: undefined,
healthCheckPath: undefined,
healthTimeout: loadBalancer.healthTimeout,
healthInterval: loadBalancer.healthInterval,
healthyThreshold: loadBalancer.healthyThreshold,
unhealthyThreshold: loadBalancer.unhealthyThreshold,
healthCheckProtocol: loadBalancer.healthCheckProtocol,
healthCheckPort: loadBalancer.healthCheckPort,
healthCheckPath: loadBalancer.healthCheckPath,
Copy link
Contributor

Choose a reason for hiding this comment

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

would like @jrsquared to take a look at this part - this seems reasonable to me (and if they're undefined, they'll get set further down in the method) but would like a little more validation from someone who's looked at this code more recently and is more familiar with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks reasonable to me. I want to pull this down and test it first though. I should have time in the next couple of days.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These need to be set during the edit, or their values aren't carried over from what was previously set (they are set by default when you create the load balancer, so setting them back to undefined here would delete whatever config had been initially saved).

subnetType: loadBalancer.subnetType,
};

Expand Down Expand Up @@ -214,7 +214,7 @@ export class AwsLoadBalancerTransformer {
region: loadBalancer.region,
loadBalancerType: 'application',
cloudProvider: loadBalancer.cloudProvider,
credentials: loadBalancer.account,
credentials: loadBalancer.account || loadBalancer.credentials,
listeners: [],
targetGroups: [],
name: loadBalancer.name,
Expand Down
2 changes: 1 addition & 1 deletion ...pts/modules/core/src/pipeline/config/stages/createLoadBalancer/createLoadBalancerStage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ module.exports = angular.module('spinnaker.core.pipeline.stage.createLoadBalance

let config = cloudProviderRegistry.getValue(selectedProvider, 'loadBalancer');
$uibModal.open({
templateUrl: config.editLoadBalancerTemplateUrl,
templateUrl: config.createLoadBalancerTemplateUrl,
controller: `${config.createLoadBalancerController} as ctrl`,
size: 'lg',
resolve: {
Expand Down
3 changes: 2 additions & 1 deletion settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ var fiatEnabled = process.env.FIAT_ENABLED === 'true' ? true : false;
var entityTagsEnabled = process.env.ENTITY_TAGS_ENABLED === 'true' ? true : false;
var debugEnabled = process.env.DEBUG_ENABLED === 'false' ? false : true;
var canaryEnabled = process.env.CANARY_ENABLED === 'true';
var infrastructureEnabled = process.env.INFRA_ENABLED === 'true' ? true : false;

window.spinnakerSettings = {
checkForUpdates: true,
Expand Down Expand Up @@ -140,7 +141,7 @@ window.spinnakerSettings = {
netflixMode: netflixMode,
chaosMonkey: chaosEnabled,
// whether stages affecting infrastructure (like "Create Load Balancer") should be enabled or not
infrastructureStages: process.env.INFRA_STAGES === 'enabled',
infrastructureStages: infrastructureEnabled,
jobs: false,
snapshots: false,
travis: false,
Expand Down
1 change: 1 addition & 0 deletions webpack.common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const HAPPY_PACK_ENV_INVALIDATE = lodash.pick(process.env, [
'DEBUG_ENABLED',
'CANARY_ENABLED',
'INF_SEARCH_ENABLED',
'INFRA_ENABLED',
]);
const HtmlWebpackPlugin = require('html-webpack-plugin');

Expand Down