Skip to content

Commit

Permalink
feat(core): move from provider version UI implementation to skins (#5080
Browse files Browse the repository at this point in the history 
)
  • Loading branch information
@danielpeach
danielpeach committed Mar 28, 2018
1 parent 987d268 commit dc6d3fd
Show file tree
Hide file tree
Showing 27 changed files with 167 additions and 165 deletions.
1 change: 1 addition & 0 deletions app/scripts/modules/core/src/account/account.service.ts
View file
Expand Up @@ -23,6 +23,7 @@ export interface IAccount {
requiredGroupMembership: string[];
type: string;
providerVersion?: string;
skin?: string;
}

export interface IAccountDetails extends IAccount {
Expand Down
8 changes: 4 additions & 4 deletions app/scripts/modules/core/src/cloudProvider/cloudProvider.module.ts
View file
@@ -1,14 +1,14 @@
import { module } from 'angular';

import { CLOUD_PROVIDER_LOGO } from './cloudProviderLogo.component';
import { VERSIONED_CLOUD_PROVIDER_SERVICE } from './versionedCloudProvider.service';
import { VERSION_SELECTOR_CTRL } from './versionSelection/versionSelector.component';
import { SKIN_SERVICE } from './skin.service';
import { SKIN_SELECTOR_CTRL } from './skinSelection/skinSelector.component';

export const CLOUD_PROVIDER_MODULE = 'spinnaker.core.cloudProvider';
module(CLOUD_PROVIDER_MODULE, [
CLOUD_PROVIDER_LOGO,
require('./cloudProviderLabel.directive').name,
require('./providerSelection/providerSelector.directive').name,
VERSION_SELECTOR_CTRL,
VERSIONED_CLOUD_PROVIDER_SERVICE,
SKIN_SELECTOR_CTRL,
SKIN_SERVICE,
]);
22 changes: 11 additions & 11 deletions app/scripts/modules/core/src/cloudProvider/cloudProvider.registry.spec.ts
View file
Expand Up @@ -103,24 +103,24 @@ describe('cloudProviderRegistry: API', function() {
});
});

describe('versioned provider configs', () => {
it('returns a value from a versioned provider config when providerVersion is specified', () => {
configurer.registerProvider('kubernetes', { name: 'kubernetes', providerVersion: 'v1' });
configurer.registerProvider('kubernetes', { name: 'kubernetes', providerVersion: 'v2' });
describe('skinned provider configs', () => {
it('returns a value from a skinned provider config when skin is specified', () => {
configurer.registerProvider('kubernetes', { name: 'kubernetes', skin: 'v1' });
configurer.registerProvider('kubernetes', { name: 'kubernetes', skin: 'v2' });

expect(configurer.$get().getValue('kubernetes', 'providerVersion', 'v1')).toBe('v1');
expect(configurer.$get().getValue('kubernetes', 'providerVersion', 'v2')).toBe('v2');
expect(configurer.$get().getValue('kubernetes', 'skin', 'v1')).toBe('v1');
expect(configurer.$get().getValue('kubernetes', 'skin', 'v2')).toBe('v2');
});

it('returns a value from the default version if providerVersion is not specified', () => {
configurer.registerProvider('kubernetes', { name: 'kubernetes', providerVersion: 'v1' });
configurer.registerProvider('kubernetes', { name: 'kubernetes', providerVersion: 'v2', defaultVersion: true });
it('returns a value from the default skin if skin is not specified', () => {
configurer.registerProvider('kubernetes', { name: 'kubernetes', skin: 'v1' });
configurer.registerProvider('kubernetes', { name: 'kubernetes', skin: 'v2', defaultSkin: true });

expect(configurer.$get().getValue('kubernetes', 'providerVersion')).toBe('v2');
expect(configurer.$get().getValue('kubernetes', 'skin')).toBe('v2');
});

// This behavior is implicitly tested in other tests, but demonstrates that the provider
// configs do not need to add a `defaultVersion` flag if there is only one config for that provider.
// configs do not need to add a `defaultSkin` flag if there is only one config for that provider.
it('behaves reasonably if a provider does not define a default config version', () => {
configurer.registerProvider('gce', { name: 'gce', key: 'value' });

Expand Down
47 changes: 24 additions & 23 deletions app/scripts/modules/core/src/cloudProvider/cloudProvider.registry.ts
View file
Expand Up @@ -12,7 +12,8 @@ export interface ICloudProviderConfig {
name: string;
logo?: ICloudProviderLogo;
providerVersion?: string;
defaultVersion?: boolean;
skin?: string;
defaultSkin?: boolean;
[attribute: string]: any;
}

Expand All @@ -25,30 +26,30 @@ class Providers {
// The tests depend on this behavior, but maybe something else does as well.
this.providers = without(
this.providers,
this.providers.find(p => p.cloudProvider === cloudProvider && p.config.providerVersion === config.providerVersion)
this.providers.find(p => p.cloudProvider === cloudProvider && p.config.skin === config.skin)
).concat([{ cloudProvider, config }]);
}

public get(cloudProvider: string, providerVersion?: string): ICloudProviderConfig {
if (providerVersion) {
const provider = this.providers.find(p => p.cloudProvider === cloudProvider && p.config.providerVersion === providerVersion);
return provider ? provider.config : this.getDefaultConfig(cloudProvider);
public get(cloudProvider: string, skin?: string): ICloudProviderConfig {
if (skin) {
const provider = this.providers.find(p => p.cloudProvider === cloudProvider && p.config.skin === skin);
return provider ? provider.config : this.getDefaultSkin(cloudProvider);
} else {
return this.getDefaultConfig(cloudProvider);
return this.getDefaultSkin(cloudProvider);
}
}

public has(cloudProvider: string, providerVersion?: string): boolean {
return !!this.get(cloudProvider, providerVersion);
public has(cloudProvider: string, skin?: string): boolean {
return !!this.get(cloudProvider, skin);
}

public keys(): string[] {
return uniq(this.providers.map(p => p.cloudProvider));
}

private getDefaultConfig(cloudProvider: string): ICloudProviderConfig {
const provider = this.providers.some(p => p.cloudProvider === cloudProvider && p.config.defaultVersion)
? this.providers.find(p => p.cloudProvider === cloudProvider && p.config.defaultVersion)
private getDefaultSkin(cloudProvider: string): ICloudProviderConfig {
const provider = this.providers.some(p => p.cloudProvider === cloudProvider && p.config.defaultSkin)
? this.providers.find(p => p.cloudProvider === cloudProvider && p.config.defaultSkin)
: this.providers.find(p => p.cloudProvider === cloudProvider);
return provider ? provider.config : null;
}
Expand All @@ -70,20 +71,20 @@ export class CloudProviderRegistry {
}
}

public getProvider(cloudProvider: string, providerVersion?: string): ICloudProviderConfig {
return this.providers.has(cloudProvider, providerVersion) ? cloneDeep(this.providers.get(cloudProvider, providerVersion)) : null;
public getProvider(cloudProvider: string, skin?: string): ICloudProviderConfig {
return this.providers.has(cloudProvider, skin) ? cloneDeep(this.providers.get(cloudProvider, skin)) : null;
}

public listRegisteredProviders(): string[] {
return Array.from(this.providers.keys());
}

public overrideValue(cloudProvider: string, key: string, overrideValue: any, providerVersion?: string) {
if (!this.providers.has(cloudProvider, providerVersion)) {
console.warn(`Cannot override "${key}" for provider "${cloudProvider}${providerVersion ? `:${providerVersion}` : ''}" (provider not registered)`);
public overrideValue(cloudProvider: string, key: string, overrideValue: any, skin?: string) {
if (!this.providers.has(cloudProvider, skin)) {
console.warn(`Cannot override "${key}" for provider "${cloudProvider}${skin ? `:${skin}` : ''}" (provider not registered)`);
return;
}
const config = this.providers.get(cloudProvider, providerVersion),
const config = this.providers.get(cloudProvider, skin),
parentKeys = key.split('.'),
lastKey = parentKeys.pop();
let current = config;
Expand All @@ -98,15 +99,15 @@ export class CloudProviderRegistry {
current[lastKey] = overrideValue;
}

public hasValue(cloudProvider: string, key: string, providerVersion?: string) {
return this.providers.has(cloudProvider, providerVersion) && this.getValue(cloudProvider, key, providerVersion) !== null;
public hasValue(cloudProvider: string, key: string, skin?: string) {
return this.providers.has(cloudProvider, skin) && this.getValue(cloudProvider, key, skin) !== null;
}

public getValue(cloudProvider: string, key: string, providerVersion?: string): any {
if (!key || !this.providers.has(cloudProvider, providerVersion)) {
public getValue(cloudProvider: string, key: string, skin?: string): any {
if (!key || !this.providers.has(cloudProvider, skin)) {
return null;
}
const config = this.getProvider(cloudProvider, providerVersion),
const config = this.getProvider(cloudProvider, skin),
keyParts = key.split('.');
let current = config,
notFound = false;
Expand Down
2 changes: 1 addition & 1 deletion app/scripts/modules/core/src/cloudProvider/index.ts
View file
Expand Up @@ -2,4 +2,4 @@ export * from './cloudProvider.registry';
export * from './CloudProviderLogo';
export * from './providerSelection/providerSelection.service';
export * from './providerService.delegate';
export * from './versionedCloudProvider.service';
export * from './skin.service';
4 changes: 2 additions & 2 deletions ...cripts/modules/core/src/cloudProvider/providerSelection/providerSelection.service.spec.ts
View file
Expand Up @@ -186,7 +186,7 @@ describe('providerSelectionService: API', () => {
let provider = '';
hasValue = true;
const k8s = fakeAccount('kubernetes');
k8s.providerVersion = 'v2';
k8s.skin = 'v2';
accounts = [k8s];
cloudProvider.registerProvider('kubernetes', config);
SETTINGS.defaultProvider = 'defaultProvider';
Expand All @@ -203,7 +203,7 @@ describe('providerSelectionService: API', () => {
let provider = '';
hasValue = true;
const k8s = fakeAccount('kubernetes');
k8s.providerVersion = 'v2';
k8s.skin = 'v2';
accounts = [k8s, fakeAccount('titus')];
cloudProvider.registerProvider('titus', config);
cloudProvider.registerProvider('kubernetes', config);
Expand Down
2 changes: 1 addition & 1 deletion app/scripts/modules/core/src/cloudProvider/providerSelection/providerSelection.service.ts
View file
Expand Up @@ -30,7 +30,7 @@ export class ProviderSelectionService {

if (filterFn) {
reducedAccounts = reducedAccounts.filter((acc: IAccountDetails) => {
return filterFn(application, acc, this.cloudProviderRegistry.getProvider(acc.cloudProvider, acc.providerVersion));
return filterFn(application, acc, this.cloudProviderRegistry.getProvider(acc.cloudProvider, acc.skin));
});
}

Expand Down
8 changes: 4 additions & 4 deletions app/scripts/modules/core/src/cloudProvider/providerService.delegate.ts
View file
Expand Up @@ -7,13 +7,13 @@ export class ProviderServiceDelegate {

constructor(private $injector: IInjectorService, private cloudProviderRegistry: any) { 'ngInject'; }

public hasDelegate(provider: string, serviceKey: string, providerVersion?: string): boolean {
const service: string = this.cloudProviderRegistry.getValue(provider, serviceKey, providerVersion);
public hasDelegate(provider: string, serviceKey: string, skin?: string): boolean {
const service: string = this.cloudProviderRegistry.getValue(provider, serviceKey, skin);
return this.$injector.has(service);
}

public getDelegate<T>(provider: string, serviceKey: string, providerVersion?: string): T {
const service = this.cloudProviderRegistry.getValue(provider, serviceKey, providerVersion);
public getDelegate<T>(provider: string, serviceKey: string, skin?: string): T {
const service = this.cloudProviderRegistry.getValue(provider, serviceKey, skin);
if (this.$injector.has(service)) {
return this.$injector.get<T>(service, 'providerDelegate');
} else {
Expand Down
48 changes: 24 additions & 24 deletions ...er/versionedCloudProvider.service.spec.ts → ...re/src/cloudProvider/skin.service.spec.ts
View file
Expand Up @@ -2,14 +2,14 @@ import { mock, IRootScopeService, IScope, IQService } from 'angular';

import { CLOUD_PROVIDER_REGISTRY } from './cloudProvider.registry';
import { ACCOUNT_SERVICE } from 'core/account/account.service';
import { VERSIONED_CLOUD_PROVIDER_SERVICE, VersionedCloudProviderService } from './versionedCloudProvider.service';
import { SKIN_SERVICE, SkinService } from './skin.service';
import { APPLICATION_MODEL_BUILDER, ApplicationModelBuilder } from 'core/application';

describe('Service: versionedCloudProviderService', () => {
let service: VersionedCloudProviderService, appBuilder: ApplicationModelBuilder, scope: IScope, $q: IQService;
describe('Service: skinService', () => {
let service: SkinService, appBuilder: ApplicationModelBuilder, scope: IScope, $q: IQService;

beforeEach((mock.module(
VERSIONED_CLOUD_PROVIDER_SERVICE,
SKIN_SERVICE,
APPLICATION_MODEL_BUILDER,
CLOUD_PROVIDER_REGISTRY,
ACCOUNT_SERVICE,
Expand All @@ -18,41 +18,41 @@ describe('Service: versionedCloudProviderService', () => {
beforeEach(
mock.inject(($rootScope: IRootScopeService,
_$q_: IQService,
versionedCloudProviderService: VersionedCloudProviderService,
skinService: SkinService,
applicationModelBuilder: ApplicationModelBuilder) => {
service = versionedCloudProviderService;
service = skinService;
appBuilder = applicationModelBuilder;
scope = $rootScope.$new();
$q = _$q_;
}
));

describe('instance provider version disambiguation', () => {
describe('instance skin disambiguation', () => {
beforeEach(() => {
spyOn(service, 'getAccounts').and.returnValue(
$q.resolve([
{ name: 'v1-k8s-account', cloudProvider: 'kubernetes', providerVersion: 'v1' },
{ name: 'v2-k8s-account', cloudProvider: 'kubernetes', providerVersion: 'v2' },
{ name: 'appengine-account', cloudProvider: 'appengine', providerVersion: 'v1' },
{ name: 'v1-k8s-account', cloudProvider: 'kubernetes', skin: 'v1' },
{ name: 'v2-k8s-account', cloudProvider: 'kubernetes', skin: 'v2' },
{ name: 'appengine-account', cloudProvider: 'appengine', skin: 'v1' },
{ name: 'gce-account', cloudProvider: 'gce' },
])
);
});

it('uses available accounts to determine provider version if possible', () => {
it('uses available accounts to determine skin if possible', () => {
const app = appBuilder.createStandaloneApplication('myApp');

service.getInstanceProviderVersion('appengine', 'my-instance-id', app).then(providerVersion => {
expect(providerVersion).toEqual('v1');
service.getInstanceSkin('appengine', 'my-instance-id', app).then(skin => {
expect(skin).toEqual('v1');
});
service.getInstanceProviderVersion('gce', 'my-instance-id', app).then(providerVersion => {
expect(providerVersion).toEqual(null);
service.getInstanceSkin('gce', 'my-instance-id', app).then(skin => {
expect(skin).toEqual(null);
});

scope.$digest();
});

it('scrapes application server groups to determine provider version if possible', () => {
it('scrapes application server groups to determine skin if possible', () => {
const app = appBuilder.createApplication('myApp', [
{
key: 'serverGroups',
Expand All @@ -74,14 +74,14 @@ describe('Service: versionedCloudProviderService', () => {
}
]);

service.getInstanceProviderVersion('kubernetes', 'my-instance-id', app).then(providerVersion => {
expect(providerVersion).toEqual('v2');
service.getInstanceSkin('kubernetes', 'my-instance-id', app).then(skin => {
expect(skin).toEqual('v2');
});

scope.$digest();
});

it('scrapes application load balancers to determine provider version if possible', () => {
it('scrapes application load balancers to determine skin if possible', () => {
const app = appBuilder.createApplication('myApp', [
{
key: 'loadBalancers',
Expand All @@ -102,14 +102,14 @@ describe('Service: versionedCloudProviderService', () => {
},
]);

service.getInstanceProviderVersion('kubernetes', 'my-instance-id', app).then(providerVersion => {
expect(providerVersion).toEqual('v2');
service.getInstanceSkin('kubernetes', 'my-instance-id', app).then(skin => {
expect(skin).toEqual('v2');
});

scope.$digest();
});

it('scrapes application load balancers\' server groups to determine provider version if possible', () => {
it('scrapes application load balancers\' server groups to determine skin if possible', () => {
const app = appBuilder.createApplication('myApp', [
{
key: 'loadBalancers',
Expand All @@ -132,8 +132,8 @@ describe('Service: versionedCloudProviderService', () => {
},
]);

service.getInstanceProviderVersion('kubernetes', 'my-instance-id', app).then(providerVersion => {
expect(providerVersion).toEqual('v2');
service.getInstanceSkin('kubernetes', 'my-instance-id', app).then(skin => {
expect(skin).toEqual('v2');
});

scope.$digest();
Expand Down

0 comments on commit dc6d3fd

Please sign in to comment.