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

Make all arguments as keyword arguments #81

Closed
wants to merge 2 commits into from
Closed

Make all arguments as keyword arguments #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81b668d
Make all args as keyword arguments
gaiksaya Nov 22, 2023
06d1db2
Fix readme typo
gaiksaya Nov 22, 2023
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ cdk synth "*" --context securityDisabled=false \
--context distVersion=2.3.0 --context serverAccessType=ipv4 --context restrictServerAccessTo=10.10.10.10/32
```

#### Sample command to set up multi-node cluster with security disabled on x64 AL2 machine
#### Sample command to set up multi-node cluster with security enabled on x64 AL2 machine

Please note that as of now we only support instances backed by Amazon Linux-2 amis.

Expand Down
8 changes: 4 additions & 4 deletions lib/infra/infra-stack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,16 @@
readonly mlNodeCount: number,
readonly dataNodeStorage: number,
readonly mlNodeStorage: number,
readonly jvmSysPropsString?: string,
readonly additionalConfig?: string,
readonly additionalOsdConfig?: string,
readonly dataEc2InstanceType: InstanceType,
readonly mlEc2InstanceType: InstanceType,
readonly use50PercentHeap: boolean,
readonly isInternal: boolean,
readonly enableRemoteStore: boolean,
readonly storageVolumeType: EbsDeviceVolumeType,
readonly customRoleArn: string
readonly customRoleArn: string,
readonly jvmSysPropsString?: string,
readonly additionalConfig?: string,
readonly additionalOsdConfig?: string,
}

export class InfraStack extends Stack {
Expand Down Expand Up @@ -147,7 +147,7 @@
}

if (props.singleNodeCluster) {
console.log('Single node value is true, creating single node configurations');

Check warning on line 150 in lib/infra/infra-stack.ts

View workflow job for this annotation

GitHub Actions / build 

Unexpected console statement
singleNodeInstance = new Instance(this, 'single-node-instance', {
vpc: props.vpc,
instanceType: singleNodeInstanceType,
Expand Down
22 changes: 11 additions & 11 deletions lib/os-cluster-entrypoint.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,30 +249,30 @@ export class OsClusterEntrypoint {
securityDisabled: security,
opensearchVersion: distVersion,
clientNodeCount: clientCount,
cpuArch,
cpuArch: cpuArch,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make any difference?
Since passed argument name and accepting parameter name is same, having parameter1: argument1 is same as parameter1. Or is it different?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think variable name as any significance in mapping (I might be wrong). This is to keep the args consistent and not worry about naming or order of arguments (positional) since the list is only going to grow from now on. Recommending to choose one for better maintenance and passing between classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right! TS is weird. The compiler is resetting in my local too. Found this: https://stackoverflow.com/questions/49798156/typescript-variable-name-as-interface-type
Do you think it is a better idea to have different variable names than the keyword arguments to be passed onto as key value pairs ? If not you can close this PR @rishabh6788

cpuType: instanceCpuType,
dataEc2InstanceType,
mlEc2InstanceType,
dataEc2InstanceType: dataEc2InstanceType,
mlEc2InstanceType: mlEc2InstanceType,
dashboardsUrl: dashboardUrl,
dataNodeCount: dataCount,
distributionUrl,
distributionUrl: distributionUrl,
ingestNodeCount: ingestCount,
managerNodeCount: managerCount,
minDistribution: minDist,
mlNodeCount: mlCount,
// @ts-ignore
securityGroup: this.securityGroup,
singleNodeCluster: isSingleNode,
dataNodeStorage,
mlNodeStorage,
dataNodeStorage: dataNodeStorage,
mlNodeStorage: mlNodeStorage,
use50PercentHeap: use50PercentHeap,
isInternal: isInternal,
enableRemoteStore: enableRemoteStore,
storageVolumeType: volumeType,
customRoleArn: customRoleArn,
jvmSysPropsString: jvmSysProps,
additionalConfig: ymlConfig,
additionalOsdConfig: osdYmlConfig,
use50PercentHeap,
isInternal,
enableRemoteStore,
storageVolumeType: volumeType,
customRoleArn,
...props,
});

Expand Down
Loading