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

Jump to bottom

[Non-Inclusive Language] Replace isDevClusterMaster with isDevClusterManager #1719

Merged
merged 1 commit into from
Jun 17, 2022
Merged

[Non-Inclusive Language] Replace isDevClusterMaster with isDevClusterManager #1719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 0 deletions packages/osd-config/src/__mocks__/env.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,7 @@ export function getEnvOptions(options: DeepPartial<EnvOptions> = {}): EnvOptions
},
isDevClusterMaster:
options.isDevClusterMaster !== undefined ? options.isDevClusterMaster : false,
isDevClusterManager:
options.isDevClusterManager !== undefined ? options.isDevClusterManager : false,
};
}
98 changes: 91 additions & 7 deletions packages/osd-config/src/__snapshots__/env.test.ts.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 41 additions & 1 deletion packages/osd-config/src/env.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ beforeEach(() => {
mockPackage.raw = {};
});

test('correctly creates default environment in dev mode.', () => {
test('correctly creates default environment in dev mode when isDevClusterMaster (deprecated) is true', () => {
mockPackage.raw = {
branch: 'some-branch',
version: 'some-version',
Expand All @@ -59,10 +59,50 @@ test('correctly creates default environment in dev mode.', () => {
getEnvOptions({
configs: ['/test/cwd/config/opensearch_dashboards.yml'],
isDevClusterMaster: true,
isDevClusterManager: false,
})
);

expect(defaultEnv).toMatchSnapshot('env properties');
expect(defaultEnv.isDevClusterManager).toBeTruthy();
Copy link
Member

@kavilla kavilla Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

line 62 says false but this is expected to be truthy this is because isDevClusterMaster is true then we expect isDevClusterManager to be true right? That's great. Can we have a test for the invert. Like isDevClusterManager is set to true. What's the expected behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

line 62 says false but this is expected to be truthy this is because isDevClusterMaster is true then we expect isDevClusterManager to be true right? That's great. Can we have a test for the invert. Like isDevClusterManager is set to true. What's the expected behavior?

Updated

});

test('correctly creates default environment in dev mode when isDevClusterManager is true', () => {
mockPackage.raw = {
branch: 'some-branch',
version: 'some-version',
};

const defaultEnv = Env.createDefault(
REPO_ROOT,
getEnvOptions({
configs: ['/test/cwd/config/opensearch_dashboards.yml'],
isDevClusterMaster: false,
isDevClusterManager: true,
})
);

expect(defaultEnv).toMatchSnapshot('env properties');
expect(defaultEnv.isDevClusterManager).toBeTruthy();
});

test('correctly creates default environment in dev mode when isDevClusterManager and isDevClusterMaster both are true', () => {
mockPackage.raw = {
branch: 'some-branch',
version: 'some-version',
};

const defaultEnv = Env.createDefault(
REPO_ROOT,
getEnvOptions({
configs: ['/test/cwd/config/opensearch_dashboards.yml'],
isDevClusterMaster: true,
isDevClusterManager: true,
})
);

expect(defaultEnv).toMatchSnapshot('env properties');
expect(defaultEnv.isDevClusterManager).toBeTruthy();
});

test('correctly creates default environment in prod distributable mode.', () => {
Expand Down
8 changes: 5 additions & 3 deletions packages/osd-config/src/env.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ import { PackageInfo, EnvironmentMode } from './types';
export interface EnvOptions {
configs: string[];
cliArgs: CliArgs;
/** @deprecated use isDevClusterManager */
isDevClusterMaster: boolean;
kavilla marked this conversation as resolved.
Show resolved Hide resolved
isDevClusterManager: boolean;
}

/** @internal */
Expand Down Expand Up @@ -110,10 +112,10 @@ export class Env {
public readonly configs: readonly string[];

/**
* Indicates that this OpenSearch Dashboards instance is run as development Node Cluster master.
* Indicates that this OpenSearch Dashboards instance is run as development Node Cluster manager.
* @internal
*/
public readonly isDevClusterMaster: boolean;
public readonly isDevClusterManager: boolean;

/**
* @internal
Expand All @@ -137,7 +139,7 @@ export class Env {

this.cliArgs = Object.freeze(options.cliArgs);
this.configs = Object.freeze(options.configs);
this.isDevClusterMaster = options.isDevClusterMaster;
this.isDevClusterManager = options.isDevClusterManager || options.isDevClusterMaster;

const isDevMode = this.cliArgs.dev || this.cliArgs.envName === 'development';
this.mode = Object.freeze<EnvironmentMode>({
Expand Down
5 changes: 3 additions & 2 deletions src/core/server/bootstrap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
*/

import chalk from 'chalk';
import { isMaster } from 'cluster';
import { isMaster as isClusterManager } from 'cluster';
import { CliArgs, Env, RawConfigService } from './config';
import { Root } from './root';
import { CriticalError } from './errors';
Expand Down Expand Up @@ -82,7 +82,8 @@ export async function bootstrap({
const env = Env.createDefault(REPO_ROOT, {
configs,
cliArgs,
isDevClusterMaster: isMaster && cliArgs.dev && features.isClusterModeSupported,
Copy link
Member

Choose a reason for hiding this comment

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

is the update to isMaster covered by a different issue/PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated! I've added alias to the 'isMaster' referenced from @types/node modules.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to change or use alias for isMaster? I think it is defined in node cluster index which is not the code we write/control. If it is something we import, do we have to rename?

Reference: https://nodejs.org/docs/latest-v14.x/api/cluster.html#cluster_cluster_ismaster

isDevClusterMaster: isClusterManager && cliArgs.dev && features.isClusterModeSupported,
isDevClusterManager: isClusterManager && cliArgs.dev && features.isClusterModeSupported,
});

const rawConfigService = new RawConfigService(env.configs, applyConfigOverrides);
Expand Down
68 changes: 66 additions & 2 deletions src/core/server/http/http_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ test('returns http server contract on setup', async () => {
});
});

test('does not start http server if process is dev cluster master', async () => {
test('does not start http server if process is dev cluster master (deprecated) or dev cluster manager', async () => {
const configService = createConfigService();
const httpServer = {
isListening: () => false,
Expand All @@ -275,7 +275,71 @@ test('does not start http server if process is dev cluster master', async () =>
const service = new HttpService({
coreId,
configService,
env: Env.createDefault(REPO_ROOT, getEnvOptions({ isDevClusterMaster: true })),
env: Env.createDefault(
REPO_ROOT,
getEnvOptions({
isDevClusterManager: true,
isDevClusterMaster: true,
})
),
logger,
});

await service.setup(setupDeps);
await service.start();

expect(httpServer.start).not.toHaveBeenCalled();
});

test('does not start http server if process is dev cluster manager', async () => {
const configService = createConfigService();
const httpServer = {
isListening: () => false,
setup: jest.fn().mockReturnValue({}),
start: jest.fn(),
stop: noop,
};
mockHttpServer.mockImplementation(() => httpServer);

const service = new HttpService({
coreId,
configService,
env: Env.createDefault(
REPO_ROOT,
getEnvOptions({
isDevClusterManager: true,
isDevClusterMaster: false,
})
),
logger,
});

await service.setup(setupDeps);
await service.start();

expect(httpServer.start).not.toHaveBeenCalled();
});

test('does not start http server if process is dev cluster master (deprecated)', async () => {
const configService = createConfigService();
const httpServer = {
isListening: () => false,
setup: jest.fn().mockReturnValue({}),
start: jest.fn(),
stop: noop,
};
mockHttpServer.mockImplementation(() => httpServer);

const service = new HttpService({
coreId,
configService,
env: Env.createDefault(
REPO_ROOT,
getEnvOptions({
isDevClusterManager: false,
isDevClusterMaster: true,
})
),
logger,
});

Expand Down
2 changes: 1 addition & 1 deletion src/core/server/http/http_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ export class HttpService
* @internal
* */
private shouldListen(config: HttpConfig) {
return !this.coreContext.env.isDevClusterMaster && config.autoListen;
return !this.coreContext.env.isDevClusterManager && config.autoListen;
}
ananzh marked this conversation as resolved.
Show resolved Hide resolved

public async stop() {
Expand Down
4 changes: 3 additions & 1 deletion src/core/server/legacy/legacy_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ describe('once LegacyService is set up in `devClusterMaster` mode', () => {
REPO_ROOT,
getEnvOptions({
cliArgs: { silent: true, basePath: false },
isDevClusterManager: false,
isDevClusterMaster: true,
})
),
Expand Down Expand Up @@ -402,7 +403,8 @@ describe('once LegacyService is set up in `devClusterMaster` mode', () => {
REPO_ROOT,
getEnvOptions({
cliArgs: { quiet: true, basePath: true },
isDevClusterMaster: true,
isDevClusterManager: true,
isDevClusterMaster: false,
})
),
logger,
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/legacy/legacy_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ export class LegacyService implements CoreService {
this.log.debug('starting legacy service');

// Receive initial config and create osdServer/ClusterManager.
if (this.coreContext.env.isDevClusterMaster) {
if (this.coreContext.env.isDevClusterManager) {
await this.createClusterManager(this.legacyRawConfig!);
} else {
this.osdServer = await this.createOsdServer(
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/plugins/plugins_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
const config = await this.config$.pipe(first()).toPromise();

let contracts = new Map<PluginName, unknown>();
const initialize = config.initialize && !this.coreContext.env.isDevClusterMaster;
const initialize = config.initialize && !this.coreContext.env.isDevClusterManager;
if (initialize) {
contracts = await this.pluginsSystem.setupPlugins(deps);
this.registerPluginStaticDirs(deps);
Expand Down
Loading