Skip to content

MONGOSH-240: log out mongosh_version to log file #244

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

Merged
merged 1 commit into from
Jul 3, 2020
Merged

MONGOSH-240: log out mongosh_version to log file #244

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/service-provider-browser/src/stitch-service-provider-browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ class StitchServiceProviderBrowser implements ServiceProvider {
async getConnectionInfo(): Promise<any> {
const buildInfo = await this.buildInfo();
const topology = await this.getTopology();
const { version } = require('../package.json');
let cmdLineOpts = null;
try {
cmdLineOpts = await this.getCmdLineOpts();
Expand All @@ -79,6 +80,7 @@ class StitchServiceProviderBrowser implements ServiceProvider {
}
const connectInfo = getConnectInfo(
'', // TODO: something more useful?
version,
buildInfo,
cmdLineOpts,
topology
Expand Down
4 changes: 4 additions & 0 deletions packages/service-provider-core/src/connect-info.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ describe('getConnectInfo', function() {
is_atlas: true,
is_localhost: false,
server_version: '3.2.0-rc2',
mongosh_version: '0.0.6',
is_enterprise: true,
auth_type: 'LDAP',
is_data_lake: false,
Expand All @@ -91,6 +92,7 @@ describe('getConnectInfo', function() {
};
expect(getConnectInfo(
ATLAS_URI,
'0.0.6',
BUILD_INFO,
CMD_LINE_OPTS,
TOPOLOGY_WITH_CREDENTIALS)).to.deep.equal(output);
Expand All @@ -101,6 +103,7 @@ describe('getConnectInfo', function() {
is_atlas: true,
is_localhost: false,
server_version: '3.2.0-rc2',
mongosh_version: '0.0.6',
is_enterprise: true,
auth_type: null,
is_data_lake: false,
Expand All @@ -114,6 +117,7 @@ describe('getConnectInfo', function() {
};
expect(getConnectInfo(
ATLAS_URI,
'0.0.6',
BUILD_INFO,
CMD_LINE_OPTS,
TOPOLOGY_NO_CREDENTIALS)).to.deep.equal(output);
Expand Down
6 changes: 4 additions & 2 deletions packages/service-provider-core/src/connect-info.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ interface ConnectInfo {
is_atlas: boolean;
is_localhost: boolean;
server_version: string;
mongosh_version: string;
server_os?: string;
server_arch?: string;
is_enterprise: boolean;
Expand All @@ -19,7 +20,7 @@ interface ConnectInfo {
uri: string;
}

export default function getConnectInfo(uri: string, buildInfo: any, cmdLineOpts: any, topology: any): ConnectInfo {
export default function getConnectInfo(uri: string, mongoshVersion: string, buildInfo: any, cmdLineOpts: any, topology: any): ConnectInfo {
const { isGenuine: is_genuine, serverName: non_genuine_server_name } =
getBuildInfo.getGenuineMongoDB(buildInfo, cmdLineOpts);
const { isDataLake: is_data_lake, dlVersion: dl_version }
Expand All @@ -36,14 +37,15 @@ export default function getConnectInfo(uri: string, buildInfo: any, cmdLineOpts:
is_atlas: getBuildInfo.isAtlas(uri),
is_localhost: getBuildInfo.isLocalhost(uri),
server_version: buildInfo.version,
node_version: process.version,
mongosh_version: mongoshVersion,
server_os,
uri,
server_arch,
is_enterprise: getBuildInfo.isEnterprise(buildInfo),
auth_type,
is_data_lake,
dl_version,
node_version: process.version,
is_genuine,
non_genuine_server_name
};
Expand Down
2 changes: 2 additions & 0 deletions packages/service-provider-server/src/cli-service-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ class CliServiceProvider implements ServiceProvider {
async getConnectionInfo(): Promise<any> {
const buildInfo = await this.buildInfo();
const topology = await this.getTopology();
const { version } = require('../package.json');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use 1 source of truth for this? Say we would start to publish only packages that are changing this may cause a sneaky bug. Looks good anyway!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did consider that, more specifically keeping the version on shell-internal-state. However, that leads to a very weird API where this particular function getConnectionInfo() would then take version as a param. This does not make sense.

Alternatively, I have also considered storing it in the cli-service-provider and browser-service-provider, which also did not seem logical.

Finally, I have considered doing this require directly in the module that puts all the connection information together, but that would lead to have to change tests in that module every time we bump the version.

This seemed like the most straightforward approach.

As for your point about publishing packages separately -- I think if we end up doing that we will likely want to split this out of the monorepo and redo our entire build process, and if we do that, this will have to be rewritten anyways.

let cmdLineOpts = null;
try {
cmdLineOpts = await this.getCmdLineOpts();
Expand All @@ -113,6 +114,7 @@ class CliServiceProvider implements ServiceProvider {
}
const connectInfo = getConnectInfo(
this.uri,
version,
buildInfo,
cmdLineOpts,
topology
Expand Down