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

fix: allow the usage of environment variables #83

Closed
wants to merge 1 commit into from

Conversation

paul-vd
Copy link

@paul-vd paul-vd commented Mar 4, 2023

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch locize-cli@7.13.2 for the project I'm working on.

In the documentation it states that if environment variables are set, they will be used if no arguments are passed or no locize config.

Additionally if these environment variables are set:

  • LOCIZE_PROJECTID or LOCIZE_PID
  • LOCIZE_API_KEY or LOCIZE_KEY
  • LOCIZE_VERSION or LOCIZE_VER
  • LOCIZE_LANGUAGE or LOCIZE_LANG or LOCIZE_LNG

But they seem to have been missing when running the CLI, I received: error: missing required argument 'apiKey'

Here is the diff that solved my problem:

diff --git a/node_modules/locize-cli/bin/locize b/node_modules/locize-cli/bin/locize
index 7facb5c..04be9fc 100755
--- a/node_modules/locize-cli/bin/locize
+++ b/node_modules/locize-cli/bin/locize
@@ -1,4 +1,5 @@
 #!/usr/bin/env node
+require('dotenv').config();
 
 const program = require('commander');
 const colors = require('colors');

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included

Related issue : #69

@adrai
Copy link
Collaborator

adrai commented Mar 4, 2023

Just tested on my local machine... reading from the environment variables works as expected.
You can also see this in the code:

const apiKey = options.apiKey || config.apiKey || process.env.LOCIZE_API_KEY || process.env.LOCIZE_KEY;

LOCIZE_API_KEY=my-key LOCIZE_PROJECTID=my-project-id locize sync

works as intended....

The dotenv module should not be needed for that. It is required to read from .env files.

So please if you have issues with the cli to read your environment variables, create a minimal reproducible example so we can investigate.

@paul-vd
Copy link
Author

paul-vd commented Mar 10, 2023

I'm not sure how a minimal reproduction would look for this issue? here are my scripts

    "i18n:download": "locize download --clean=true --path=./public/locales",
    "i18n:sync": "locize sync --path=./public/locales --dry=true"

and my env

LOCIZE_PROJECTID="my-project-id"
LOCIZE_API_KEY="my-api-key"
LOCIZE_LANGUAGE="en-GB"
LOCIZE_VERSION="latest"

I think this is really specific to a developers local environment (machine), and the dotenv ensures that it will retrieve the environment variables, kind of same reason why a lot of people use https://www.npmjs.com/package/cross-env to support dev across multiple platforms, and it's good if packages have the same mentality.

Anyhow, I patched it for our environments to include the dotenv, feel free to close this if you feel that it "should" work, although does not in every instance 😆 #PEBKAC?

@adrai
Copy link
Collaborator

adrai commented Mar 10, 2023

If you could provide an example repository or a Docker image or similar that would be awesome.
I like to understand where the problem is, before "fixing" it.

@paul-vd
Copy link
Author

paul-vd commented Mar 11, 2023

@adrai here is a reproduction with a docker example, it does the same in the docker,

git clone https://github.com/paul-vd/locize-cli-reproduction &&
cd locize-cli-reproduction &&
docker build .

Here is the result
image

After adding the patch that I have done, it works correctly, you can test this by uncommenting the line 12 in the Docker file and running docker build . again, you should then see Project with id "my-project-id" not found! which means it reads the environment variables correctly.

adrai added a commit that referenced this pull request Mar 11, 2023
@adrai
Copy link
Collaborator

adrai commented Mar 11, 2023

@paul-vd We've just published v7.14.0 that should also respect the default .env file, but we've implemented it in a different way. Can you check if that's ok for you?

@stale
Copy link

stale bot commented Mar 18, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 18, 2023
@paul-vd
Copy link
Author

paul-vd commented Mar 21, 2023

@paul-vd We've just published v7.14.0 that should also respect the default .env file, but we've implemented it in a different way. Can you check if that's ok for you?

Looks good to me! I will close this one, thanks!

@paul-vd paul-vd closed this Mar 21, 2023
@paul-vd paul-vd deleted the fix-environment-variables branch March 21, 2023 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants