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

Added greater granularity for boolean environment variables #511

Merged

Conversation

alessandrodetta
Copy link
Contributor

@alessandrodetta alessandrodetta commented Feb 19, 2024

Changes Proposed

See suggestion in #510.

I preferred to issue this pull request as draft because this is my first open source pull request.

I will suggest in the future anyway to add implementation for some kind of structure (maybe a map?) to store parsed environment variables for both boolean and strings (in already created env.go) so that the environment variables are parsed for their correct values in one single point in code and not sparsely in different files as it is now and also so that reading multiple times their values doesn't involve more calls to the OS.

Furthermore, somehow I would also suggest integrating the management of program options into this reasoning, in order to clearer management since in some cases there is a relationship between the two.

Check List

Copy link
Collaborator

@undera undera left a comment

Choose a reason for hiding this comment

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

Looks great @alessandrodetta !
The only ask is to move utility function to a more appropriate place, see my comments.

pkg/dashboard/env/env.go Outdated Show resolved Hide resolved
pkg/dashboard/env/env.go Outdated Show resolved Hide resolved
@undera
Copy link
Collaborator

undera commented Feb 19, 2024

As for CI failure, try merging from master, I have updated golang version to 1.21 there

@alessandrodetta alessandrodetta marked this pull request as ready for review February 20, 2024 11:16
@alessandrodetta
Copy link
Contributor Author

alessandrodetta commented Feb 20, 2024

@undera Probably you got notified, but I have updated the solution with the changes you reviewed. I have rebased the feature branch so that now is on top of the current main branch so that I don't need to re-create a PR. Your idea was to create a new PR with my changes from main itself or like this is ok?

@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 30.18%. Comparing base (3d7f907) to head (4aeec65).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
pkg/dashboard/server.go 0.00% 2 Missing ⚠️
pkg/dashboard/api.go 0.00% 0 Missing and 1 partial ⚠️
pkg/dashboard/objects/data.go 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #511      +/-   ##
==========================================
+ Coverage   29.72%   30.18%   +0.46%     
==========================================
  Files          10       10              
  Lines        1329     1335       +6     
==========================================
+ Hits          395      403       +8     
+ Misses        893      891       -2     
  Partials       41       41              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@undera
Copy link
Collaborator

undera commented Feb 20, 2024

Please do another pull from master, there is small issue with static check setup in CI

@undera undera merged commit b5bed10 into komodorio:main Feb 22, 2024
4 checks passed
@undera
Copy link
Collaborator

undera commented Feb 22, 2024

Awesome! Thanks!

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

Successfully merging this pull request may close these issues.

Boolean environment variables: incoherence and ambiguity
3 participants