Configure agent nodes based on jenkins instance type #513
Conversation
Signed-off-by: Divya Madala <divyaasm@amazon.com>
Divyaasm
requested review from
peterzhuamazon,
gaiksaya,
rishabh6788,
zelinh and
prudhvigodithi
as code owners
lib/ci-stack.ts
Outdated
| @@ -52,6 +52,8 @@ export interface CIStackProps extends StackProps { | |||
| readonly enableViews?: boolean; | |||
| /** Use Production Agents */ | |||
| readonly useProdAgents?: boolean; | |||
| /** Specify jenkins instance type */ | |||
| readonly jenkinsType?: string; | |||
There was a problem hiding this comment.
Nit: jenkinsDeploymentType?
There was a problem hiding this comment.
Or jenkinsInstanceType, Which name would you recommend?
|
|
||
| public getRequiredAgentNodes(type: string): any[] { | ||
| return Object.keys(this) | ||
| .filter((key) => { |
There was a problem hiding this comment.
If I understand correctly we are just checking if default exists in the key name and then return the list of keys.
To make it extendable, do you think it will be better to add another label to workerLabelString and use that to filter out agent nodes per use case?
There was a problem hiding this comment.
We are doing similar thing with jenkinsType variable.
If jenkinsType value is not passed we assume it's default(this option is mostly used for testing), in all other cases we retrieve the agent nodes list with the value passed in jenkinsType(Ex: benchmark, gradle). But for BTR we need to exclude the agentnode names having the values ('default', 'benchmark', 'gradle').
The Drawback is agentnodes used by BTR instance cannot be differentiated from their names like we do with benchmark and gradle. We can think of updating agentnodes names used by BTR with a key_name(RELEASE) if it's straightforward.
There was a problem hiding this comment.
I think since we have a defined number of instance type, better have a check, may be enums, to verify if a valid instance type name is passed.
If invalid name is passed then throw an error.
There was a problem hiding this comment.
Agreed. Have enums for instance types. Also what is BTR? 😅
Is it build/test/release?
Signed-off-by: Divya Madala <divyaasm@amazon.com>
Signed-off-by: Divya Madala <divyaasm@amazon.com>
| @@ -28,6 +28,13 @@ import { JenkinsSecurityGroups } from './security/ci-security-groups'; | |||
| import { JenkinsWAF } from './security/waf'; | |||
| import { FineGrainedAccessSpecs } from './compute/auth-config'; | |||
|
|
|||
| enum DeploymentType { | |||
| BTR='BTR', | |||
There was a problem hiding this comment.
Just to clarify for external community.
| BTR='BTR', | |
| BTR='BTR', // Build Test Release |
| @@ -53,6 +60,8 @@ export interface CIStackProps extends StackProps { | |||
| readonly enableViews?: boolean; | |||
| /** Use Production Agents */ | |||
| readonly useProdAgents?: boolean; | |||
| /** Specify jenkins instance type */ | |||
| readonly jenkinsInstanceType?: string; | |||
There was a problem hiding this comment.
Let's keep it constant across the code. Either deploymentType or JenkinsInstanceType.
Also maybe the type should be string | DeploymentType
| // eslint-disable-next-line no-console | ||
| console.warn('Please note that if you have decided to use the provided production jenkins agents then ' | ||
| // eslint-disable-next-line no-console | ||
| console.warn('Please note that if you have decided to use the provided production jenkins agents then ' |
There was a problem hiding this comment.
Isn't the conditional useProdAgents required here?
The warning was only relevant if the ProdAgents parameter is enabled.
| @@ -23,7 +23,7 @@ export interface AgentNodeProps { | |||
| amiId: string; | |||
| instanceType: string; | |||
| customDeviceMapping: string; | |||
| workerLabelString: string; | |||
| workerLabelString: string[]; | |||
There was a problem hiding this comment.
How about make it [string, DeploymentType]. This way it will by default only accept the specified enum values.
Description
Changes to include the agent nodes required for a jenkins instance type taking the values Bulid,Test,Release, benchmark, gradle.
This implentation brings up all agent nodes for type BTR, future enhancement will only bring up B/T/R related agent nodes only
Issues Resolved
#509
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.