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

Remove viper for cleanenv #258

Merged
merged 6 commits into from
Oct 11, 2022
Merged

Remove viper for cleanenv #258

merged 6 commits into from
Oct 11, 2022

Conversation

lzap
Copy link
Member

@lzap lzap commented Oct 7, 2022

Complete rewrite of our config handling. Configuration is now done via configs/api.env (pbapi), configs/worker.env (pbworker) and configs/test.env (integration tests). There is also configs/example.env file which is generated via a new makefile target generate-example-config.

Everything, literally everything is now defined in a single file internal/config/config.go - the whole structure, types, env variable name, default value and also optionally description.

ACTION NEEDED: New configuration needs to be deployed for development setups. There should be no changes needed for Clowder, except if we have a typo in an environmental variable.

Do not merge, this is the first cut. I will fix bugs and after review I would like to do few more TODO comments which is a small refactoring which leads from my review of all values. I want to do this in a single PR so people don't need to update configuration multiple times as I want to rename and move few around.

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

The "Rename clowder service to 'api'" commit was pushed accidentally, will remove.

@lzap lzap force-pushed the viper-bye branch 2 times, most recently from 6302a28 to 41249ef Compare October 10, 2022 07:24
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

I wonder why We need different config file for api/worker, I guess most configuration in development is identical, right?

But I'm fine eighter way, it just means I need to define my DB three times, its inconvenient, but I can live with it 🤷

Apart of that 💘 it

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

Please do not merge, still working on refactoring those useless config and improving some others.

Well it seems logical to me, in clowder env you also have separate configs so I thought it's a reasonable to have this for development too. I can implement fallback, if worker.env does not exist load api.env.

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

I pushed a new version which removes EXIT_ON_PANIC (it was useless the app should never exit) and removes environment in favour or helper methods IsClowder and IsProductionClowder. The idea is, we only seem to need to check if the app is running in clowder environment or not. In one case tho (error when HTTP proxy is used) we do want to allow this in ephemeral environment because that's how IQE connects to sources. So the second helper is a WIP to check that, clowder should provide some kind of environment name I hope (there is a variable I want to test).

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

I will push new commits from now on, sorry, I haven't realized you started reviewing. The patch is smaller than I expected tho.

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

Just to let you know, those amended commits - I am just trying to break the CI so I can actually see the value in clowder.Metadata.Env variable, I guess it could be something like "ephemeral" but I need to confirm this :)

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

Ah we do NOT use HTTP proxy when in clowder, so it's even more simple, I do not need any IsProductionClowder function.

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

Ok removed, @ezr-ondrej how about this? I will only convert logger to string and heartbeat to time.Duration in separate commits.

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Nice, great refactoring, much easier to read then viper :)
The files make sense, I'd expect one main and then per-process, but api is kinda the main one, right?

Tho shouldn't the api go first in the calls? now if the var is set in worker and api, the api will win for worker, right? :)

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

Yeah I refactored and changed the order, initially I thought it is not possible to override values but looks like it does work.

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

Done, this is now ready for the final review and testing. Had to rebase one more time, there was a typo configs/api.env vs config/api.env (singular). I really think using singular is more appropriate, this was not my first time I screw this up.

If you want, I can rename the directory in another commit since people will have to update their configuration files anyway, it will be more disturbing change tho (makefile changes likely). Not a huge deal tho, two dozens of lines.

@ezr-ondrej
Copy link
Member

ezr-ondrej commented Oct 10, 2022

Not a huge deal tho, two dozens of lines.

If it's not as big, I'd be for singular as well... I've messed up few times already as well

EDIT: Just keep in mind configs came from the "common go repo structure"

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

EDIT: Just keep in mind configs came from the "common go repo structure"

Yeah but sources-api-go use config, I feel like config is more appropriate.

@lzap
Copy link
Member Author

lzap commented Oct 10, 2022

Added a new commit with renamed configuration directory.

@ezr-ondrej
Copy link
Member

Needs rebase, but I wonder how we should anounce these breaking changes to devel env? 🤔 shall we compose some list of recent changes in the repo? So devs who were away for a while can go to that list?
It could be simple list that links the PR for details ....

@lzap
Copy link
Member Author

lzap commented Oct 11, 2022

Will do, please merge this one first before these:

There will be conflicts and this one is pain to rebase (multiple commits).

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

The app starts, connects to DB and conects to AWS.
Thanks 👍 nice cleanup :)

@ezr-ondrej ezr-ondrej merged commit 05862f4 into RHEnVision:main Oct 11, 2022
@lzap lzap deleted the viper-bye branch October 11, 2022 10:20
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.

2 participants