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.

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

feat: add support for loading dotenv files #73

Closed
wants to merge 5 commits into from

Conversation

Lehoczky
Copy link
Contributor

@Lehoczky Lehoczky commented Feb 26, 2024

Closes #56

This PR adds a new option to the CLI, --env, which specifies which env file to load before a command is executed.

@Lehoczky Lehoczky marked this pull request as draft February 26, 2024 19:23
@Lehoczky Lehoczky marked this pull request as ready for review February 26, 2024 19:55
Comment on lines +142 to +153
if (process.env.TOLGEE_API_KEY) {
setOptionValue('apiKey', process.env.TOLGEE_API_KEY);
}
if (process.env.TOLGEE_API_URL) {
setOptionValue('apiUrl', parseUrlArgument(process.env.TOLGEE_API_URL));
}
if (process.env.TOLGEE_PROJECT_ID) {
setOptionValue(
'projectId',
parseProjectId(process.env.TOLGEE_PROJECT_ID)
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unsure how to feel about that. I'm aware we can't fetch env variables be fetched before because we need to get --env, however doing manually the env checks does duplicate the places where it needs to be handled and is prone to desync...

Iterating over the list of available options stored in program is probably better

Copy link
Contributor Author

@Lehoczky Lehoczky Feb 28, 2024

Choose a reason for hiding this comment

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

I can do a followup refactoring PR to handle loading the configuration in one place, potentially with a schema library, like zod, which would further simplify things.

The reason I did not iterate here, because different options need different parsing function, like the TOLGEE_PROJECT_ID needs the parseProjectId().

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the parse function already stored and accessible via the list of available options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you be able to show me a code snippet on how we could improve this function? I might just be slow today, but it would help me for sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible to get the list of options that can be specified on a given command using the options field; considering this I wonder if it's not possible to use this (along with parent since commands are structured in a tree-like structure) to get all the options that can be specified, and check if the option can use an env variable, if so use its parser and its name() to populate the command options - see this

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 haven't find a way to iterate over the options and set the env variables that way. It is not possible to get whether an option can use an env variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello, sorry for the late reply, I unfortunately got sick 🤧

I was able to iterate over all the options using this snippet:

function processCommand (command: Command) {
  for (const option of command.options) {
    console.log(option.attributeName(), option.envVar)
  }

  if (command.parent) {
    processCommand(command.parent)
  }
}

@Lehoczky Lehoczky closed this Aug 20, 2024
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.

Native support for .env files
2 participants