Skip to content

[Unified Recorder] Load the env file by default#19139

Merged
HarshaNalluru merged 7 commits intoAzure:mainfrom
HarshaNalluru:harshan/load-env-by-default
Dec 10, 2021
Merged

[Unified Recorder] Load the env file by default#19139
HarshaNalluru merged 7 commits intoAzure:mainfrom
HarshaNalluru:harshan/load-env-by-default

Conversation

@HarshaNalluru
Copy link
Copy Markdown
Contributor

@HarshaNalluru HarshaNalluru commented Dec 10, 2021

As the title says, loads the .env file for the packages by default.

Fixes #19140

Copy link
Copy Markdown
Member

@timovv timovv left a comment

Choose a reason for hiding this comment

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

Looks good.

Getting a bit of deja vu here :)

export { relativeRecordingsPath } from "./utils/relativePathCalculator";
export { SanitizerOptions, RecorderStartOptions } from "./utils/utils";
export { NoOpCredential } from "./recorderAADCredential";
export { env } from "@azure-tools/test-recorder";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need to export env from the old recorder?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Think this should be exporting from ./utils/env instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can redeclare env.

import * as dotenv from "dotenv";

// Initialize the environment
dotenv.config();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how is this executed? Is the intent that users do import "@azure-tools/recorder-new" to get this loaded?

Copy link
Copy Markdown
Contributor Author

@HarshaNalluru HarshaNalluru Dec 10, 2021

Choose a reason for hiding this comment

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

yes, that's the intention.
Or when you import anything from recorder-new

Copy link
Copy Markdown
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

Looks good! Nice to not having to load env by myself

@HarshaNalluru HarshaNalluru requested a review from scbedd December 10, 2021 21:45
Copy link
Copy Markdown
Member

@witemple-msft witemple-msft left a comment

Choose a reason for hiding this comment

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

LGTM

@HarshaNalluru HarshaNalluru merged commit 6bf749b into Azure:main Dec 10, 2021
@HarshaNalluru HarshaNalluru deleted the harshan/load-env-by-default branch December 10, 2021 23:00

// In the browser, we load the env variables with the help of karma.conf.js

export const env = (window as any).__env__;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this throw with a descriptive message if karma config did not load the env vars?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This file shouldn't know if the karma.conf has loaded the variables or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If a certain variable is undefined, we'll know from the test failure anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Unified Recorder] dotenv - load it by default from recorder-new

6 participants