Configure agent nodes based on jenkins instance type #513
Conversation
Signed-off-by: Divya Madala <[email protected]>
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?
Signed-off-by: Divya Madala <[email protected]>
Signed-off-by: Divya Madala <[email protected]>
lib/ci-stack.ts
Outdated
| @@ -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 |
lib/ci-stack.ts
Outdated
| @@ -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
lib/ci-stack.ts
Outdated
| // 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.
There was a problem hiding this comment.
We already have a check for the enum values
else if (!Object.values(DeploymentType).includes(jenkinsInstanceType as DeploymentType)) {
throw new Error(Invalid jenkinsInstanceType value: ${jenkinsInstanceType});
and we have hardcoded the DeploymentType for each agent property anyway
workerLabelString: ['Jenkins-Agent-AL2023-X64-C54xlarge-Single-Host', 'BTR'],
Do we still require to change the type @gaiksaya?
There was a problem hiding this comment.
Addressed all other comments in the latest commit @gaiksaya
There was a problem hiding this comment.
The condition will only be checked if the given value is undefined as per this code: https://github.com/opensearch-project/opensearch-ci/pull/513/files#diff-4b6fe4122d26e45b42b65d74533746571fed0807fe76bb2df2ea33b75227e004R130-R134
We want to check despite of that
There was a problem hiding this comment.
Synced up offline. The if else block does the needful. We can iterate on this logic later.
Signed-off-by: Divya Madala <[email protected]>
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.