Skip to content

Commit

Permalink
fix comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Lizaga Anadon committed Jan 19, 2023
1 parent 34da934 commit e37e541
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 26 deletions.
62 changes: 39 additions & 23 deletions packages/@aws-cdk/aws-apprunner/lib/service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,9 +467,9 @@ export interface ImageConfiguration {
* Environment variables that are available to your running App Runner service.
*
* @default - no environment variables
* @deprecated use environmentVariables()
* @deprecated use environmentVariables.
*/
readonly environment?: { [key: string]: string };
readonly environment?: { [key: string]: string }

/**
* Environment variables that are available to your running App Runner service.
Expand Down Expand Up @@ -718,7 +718,7 @@ export interface CodeConfigurationValues {
* The environment variables that are available to your running App Runner service.
*
* @default - no environment variables.
* @deprecated use environmentVariables()
* @deprecated use environmentVariables.
*/
readonly environment?: { [key: string]: string };

Expand Down Expand Up @@ -918,28 +918,29 @@ export class Service extends cdk.Resource {
private source: SourceConfig;

/**
* Environment variables for this service
* @deprecated use environmentVariables()
* Environment variables for this service.
*
* @deprecated use environmentVariables.
*/
readonly environment?: { [key: string]: string } = {};
readonly environment: { [key: string]: string } = {};

/**
* Environment variables for this service
* Environment variables for this service.
*/
private environmentVariables?: { [key: string]: string } = {};
private environmentVariables: { [key: string]: string } = {};

/**
* Environment secrets for this service
* Environment secrets for this service.
*/
private environmentSecrets?: { [key: string]: Secret; };
private environmentSecrets: { [key: string]: Secret; } = {};

/**
* Environment secrets for this service
* Environment secrets for this service.
*/
private readonly secrets: EnvironmentSecret[] = []

/**
* Environment variables for this service
* Environment variables for this service.
*/
private readonly variables: EnvironmentVariable[] = []

Expand Down Expand Up @@ -988,7 +989,7 @@ export class Service extends cdk.Resource {
this.props.accessRole ?? this.generateDefaultRole() : undefined;

// generalte an IAM role only when environmentSecrets has values and props.instanceRole is undefined
this.instanceRole = (this.environmentSecrets && !this.props.instanceRole) ?
this.instanceRole = (Object.keys(this.environmentSecrets).length > 0 && !this.props.instanceRole) ?
this.createInstanceRole() : this.props.instanceRole;

if (this.source.codeRepository?.codeConfiguration.configurationSource == ConfigurationSourceType.REPOSITORY &&
Expand Down Expand Up @@ -1034,7 +1035,7 @@ export class Service extends cdk.Resource {
/**
* This method adds an environment variable to the App Runner service.
*/
public addEnvironment(name: string, value: string) {
public addEnvironmentVariable(name: string, value: string) {
this.variables.push({ name: name, value: value });
}

Expand Down Expand Up @@ -1076,16 +1077,31 @@ export class Service extends cdk.Resource {
return accessRole;
}

private getEnvironmentSecrets(): { [key: string]: Secret } | undefined {
return this.source.codeRepository?.codeConfiguration.configurationValues?.environmentSecrets ||
private getEnvironmentSecrets(): { [key: string]: Secret } {
let secrets = this.source.codeRepository?.codeConfiguration.configurationValues?.environmentSecrets ??
this.source.imageRepository?.imageConfiguration?.environmentSecrets;

return secrets || {};
}

private getEnvironmentVariables(): { [key: string]: string } | undefined {
return this.source.codeRepository?.codeConfiguration.configurationValues?.environmentVariables ||
this.source.codeRepository?.codeConfiguration.configurationValues?.environment ||
this.source.imageRepository?.imageConfiguration?.environmentVariables ||
this.source.imageRepository?.imageConfiguration?.environment;
private getEnvironmentVariables(): { [key: string]: string } {
let codeEnv = [
this.source.codeRepository?.codeConfiguration.configurationValues?.environmentVariables,
this.source.codeRepository?.codeConfiguration.configurationValues?.environment,
];
let imageEnv = [
this.source.imageRepository?.imageConfiguration?.environmentVariables,
this.source.imageRepository?.imageConfiguration?.environment,
];

if (codeEnv.every(el => el !== undefined) || imageEnv.every(el => el !== undefined)) {
throw new Error([
'You cannot set both \'environmentVariables\' and \'environment\' properties.',
'Please only use environmentVariables, as environment is deprecated.',
].join(' '));
}

return codeEnv.find(el => el !== undefined) || imageEnv.find(el => el !== undefined) || {};
}

private renderAuthenticationConfiguration(): AuthenticationConfiguration {
Expand Down Expand Up @@ -1121,7 +1137,7 @@ export class Service extends cdk.Resource {
}

private renderEnvironmentVariables(): EnvironmentVariable[] | undefined {
if (this.environmentVariables) {
if (Object.keys(this.environmentVariables).length > 0) {
for (const [key, value] of Object.entries(this.environmentVariables)) {
if (key.startsWith('AWSAPPRUNNER')) {
throw new Error(`Environment variable key ${key} with a prefix of AWSAPPRUNNER is not allowed`);
Expand All @@ -1135,7 +1151,7 @@ export class Service extends cdk.Resource {
}

private renderEnvironmentSecrets(): EnvironmentSecret[] | undefined {
if (this.environmentSecrets && this.instanceRole) {
if (Object.keys(this.environmentSecrets).length > 0 && this.instanceRole) {
for (const [key, value] of Object.entries(this.environmentSecrets)) {
if (key.startsWith('AWSAPPRUNNER')) {
throw new Error(`Environment secret key ${key} with a prefix of AWSAPPRUNNER is not allowed`);
Expand Down
28 changes: 25 additions & 3 deletions packages/@aws-cdk/aws-apprunner/test/service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as ecr_assets from '@aws-cdk/aws-ecr-assets';
import * as iam from '@aws-cdk/aws-iam';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import * as ssm from '@aws-cdk/aws-ssm';
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
import * as cdk from '@aws-cdk/core';
import * as apprunner from '../lib';

Expand Down Expand Up @@ -60,7 +61,7 @@ test('custom environment variables and start commands are allowed for imageConfi
}),
});

service.addEnvironment('SECOND_ENVIRONEMENT_VARIABLE', 'second test value');
service.addEnvironmentVariable('SECOND_ENVIRONEMENT_VARIABLE', 'second test value');

// THEN
// we should have the service
Expand Down Expand Up @@ -235,7 +236,7 @@ test('custom environment variables and start commands are allowed for imageConfi
}),
});

service.addEnvironment('SECOND_ENVIRONEMENT_VARIABLE', 'second test value');
service.addEnvironmentVariable('SECOND_ENVIRONEMENT_VARIABLE', 'second test value');

// THEN
// we should have the service
Expand Down Expand Up @@ -633,7 +634,7 @@ test('create a service with github repository - buildCommand, environment and st
}),
});

service.addEnvironment('SECOND_ENVIRONEMENT_VARIABLE', 'second test value');
service.addEnvironmentVariable('SECOND_ENVIRONEMENT_VARIABLE', 'second test value');

// THEN
// we should have the service with the branch value as 'main'
Expand Down Expand Up @@ -950,4 +951,25 @@ test('specifying a vpcConnector should assign the service to it and set the egre
],
VpcConnectorName: 'MyVpcConnector',
});
});

testDeprecated('Using both environmentVariables and environment should throw an error', () => {
const app = new cdk.App();
const stack = new cdk.Stack(app, 'demo-stack');

expect(() => {
new apprunner.Service(stack, 'DemoService', {
source: apprunner.Source.fromEcrPublic({
imageConfiguration: {
environmentVariables: {
AWSAPPRUNNER_FOO: 'bar',
},
environment: {
AWSAPPRUNNER_FOO: 'bar',
},
},
imageIdentifier: 'public.ecr.aws/aws-containers/hello-app-runner:latest',
}),
});
}).toThrow(/You cannot set both \'environmentVariables\' and \'environment\' properties./);
});

0 comments on commit e37e541

Please sign in to comment.