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: new environment variable (storage path) #2993

Merged
merged 6 commits into from Feb 14, 2022
Merged

Conversation

osher
Copy link
Contributor

@osher osher commented Feb 14, 2022

Follow up of #2199 in v5

@juanpicado
Copy link
Member

Run yarn format, everything else is pretty much ok.

@juanpicado juanpicado changed the title Migrate PR#2199->master into the 5.x branch feat: new environment variable (storage path) Feb 14, 2022
@osher
Copy link
Contributor Author

osher commented Feb 14, 2022

I'm working online using github web-ui, I did not even check out the repo...
I hoped the CI will tell me what's wrong so I can fix it - perhaps you can help me?

@juanpicado
Copy link
Member

I'm working online using github web-ui, I did not even check out the repo... I hoped the CI will tell me what's wrong so I can fix it - perhaps you can help me?

Done, it was hard to see on this UI

2022-02-14_20-55

@juanpicado juanpicado added this to the 5.x milestone Feb 14, 2022
@juanpicado juanpicado added this to In Review in Verdaccio Stable via automation Feb 14, 2022
@osher
Copy link
Contributor Author

osher commented Feb 14, 2022

gotcha! thanks :)

Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

Thanks @osher you 🎸

@juanpicado juanpicado merged commit 681dc82 into verdaccio:5.x Feb 14, 2022
Verdaccio Stable automation moved this from In Review to Closed Feb 14, 2022
@osher
Copy link
Contributor Author

osher commented Feb 14, 2022

nonono - thank you.

So, the merge to 5.x should result with a bumped version and a verdaccio/verdaccio:5.5.3 docker, or there's one more step?

P.S
you're fun to work with. that gave me appetite.
maybe I'll check out that repo one day and overcome my TS fude ...but not today 😛 🤷

@juanpicado
Copy link
Member

I need to check what's happening here #2984 and release only takes 1 min on my side.

@osher
Copy link
Contributor Author

osher commented Feb 15, 2022

It's ok, I saw the situation, we'll wait patiently.

Meanwhile - we found verdaccio:5.x-next that was published automatically, so no urgency

keep up the good work!

@juanpicado
Copy link
Member

Oh yeah, :) I forgot to mention that. Let me know if works fine for you so far.

@juanpicado
Copy link
Member

Released https://github.com/verdaccio/verdaccio/releases/tag/v5.6.0

@osher
Copy link
Contributor Author

osher commented Feb 16, 2022

The next level is an env-var to control the log-path...
How about VERDACCIO_LOG_PATH? Do you have something in the sleeve for that?
Like before - if you point me the way and I find it feasible , I just might take it 👍

BTW - if I may digress,
If you're already on a v6 deep-refactoring - what I usually do is a module that merges values from baked-in defaults, config files, env-vars and CLI switches which merge into a single settings model, cascading each other by the aforementioned order, so that nowhere in the app accesses process or open config files except this module, and every other part of the app consumes the settings from it.
e.g.

VERDACCIO_log_level=info VERDACCIO_log_path=over/there/log.txt verdaccio
 -  or -
verdaccio --log.level info log.path over/there/log.txt --storage /verdaccio/strotage --web.title ci-npm

If I'm smart enough to design the config-tree without any arrays (which you have!) - I can inject any value anywhere in the tree using CLI switches or env-vars and keep things readable. It's VERY useful for docker containers and ad-hock executions. make it feel like zero-config even when you actually pass the configs to CLI switches...
Heck! look at traefik - they consume configs from all the above - plus from docker tags...
The CLI switches part comes almost for free with minimist and lodash.merge.
An implementations for the env-vars part is either config (which basically does more).
But I usually prefer slimmer implementations like:

const set = require('lodash.set'), envCfg = {};
Object.keys(process.env)
.filter(k => k.startsWith('VERDACCIO_'))
.forEach(k => set(envCfg, k.slice(10).replace(/_/g, '.'), process.env[k]));

Mind that it requires tollerance for coresion of number from strings (because env-vars and cli-switches are by definition strings).
Mind that such an open approach is exploitable. Some end-result validation is required before throwing that in the final config. Nothing that yup won't cover - I saw you use it in packages/config.

Just saying, for your consideration 😀

--

Oh. Right. Another trick I've seen but never used myself is support an env-var like VERDACCIO_CONFIG which is expected to be a JSON expression that is deep-merged into the configuration (before merging the cli-switches that may cascade it further).
Less readable and hence less-useful than the previous way, but I've seen it embraced well by ops.
This way however, does not have the problem of string coersion.

--

Mmm. baybe this is not the place for such a discussion. LMK if you want me to put it as issues

@juanpicado
Copy link
Member

ok, message received :) let me some days to digest, these days I'm busy here https://nodecongress.com/ as speaker 😊

@osher
Copy link
Contributor Author

osher commented Feb 17, 2022

well!! good luck!

can you just say if you have something up the sleve for an env for the log path?

@juanpicado
Copy link
Member

juanpicado commented Feb 17, 2022

If I'm smart enough to design the config-tree without any arrays (which you have!)

In v6 we don't have arrays anymore for logs, I've removed them because does not make sense nowadays. Anyway your idea seems something I've heard #1681 before, I also into the idea that env variables helps a lot in eg: Docker etc

I had others ideas that might conflict with this one, like use a object schema to validate the config file (like yarn +v2 is doing with .yarnc.yaml file but nothing definitive yet, at leas for next major.

Let me digest more this when my brain get more relaxed. Feel free to remind me over time, I usually get distractec with so many open topics, or maybe better open a discussion so I can tag it correctly.

@edclement
Copy link
Collaborator

Just my 2 cents... everything being discussed here related to configuration seems like a perfect use case for using the config package. You can specify config by file, environmental variable, command line, or all of the above and it handles the merging.

@osher
Copy link
Contributor Author

osher commented Feb 20, 2022

@edclement Thanks for the comment 😄

I did mention the config pacakge above.
however, AFAIK it's doing only half of the story as it does not handle CLI-arguments. It's also does a lot more then necessary with it's local-cascading model of the hostname, instance name, env (what they call deployment) and it's approach towards env-vars is tedious: you have to map explicitly every key to it's cascading env-var.
It does however support coersion from strings.

So whenever I use it I combine it with minimist which is the actual parser behind yargs (like config, yargs is doing much more with custom-commands and message formatting). The more I understood that envs hosts and instance-number belong to the "old-world", I moved away to slimer mechanisms like require-yml and take the extra-mile by myself.

But that's just me and my fanatic approach towards slim deliveries... both yargs and config are great packages I was pushing for a long while 👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants