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

Correctly prefer datasource env var over file content #3419

Closed

Conversation

6543
Copy link
Member

@6543 6543 commented Feb 20, 2024

close #3404

fixes #3389

@6543 6543 added bug Something isn't working server labels Feb 20, 2024
@6543 6543 changed the title Woodpecker database datasource file Woodpecker preferred datasource file over database string Feb 20, 2024
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (364d708) 36.36% compared to head (4d37a15) 36.35%.

Files Patch % Lines
cmd/server/setup.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3419      +/-   ##
==========================================
- Coverage   36.36%   36.35%   -0.02%     
==========================================
  Files         228      228              
  Lines       15401    15407       +6     
==========================================
  Hits         5601     5601              
- Misses       9397     9403       +6     
  Partials      403      403              

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

@smainz
Copy link
Contributor

smainz commented Feb 21, 2024

That was option 5 :-), which I did not want to propose in #3404, due to consistency reasons. Now there are two different precendence rules for environment vars in place.

You still need to fix the documentation here:

Alternatively use docker-secrets. As it may be difficult to use docker secrets for environment variables woodpecker allows to read sensible data from files by providing a `*_FILE` option of all sensible configuration variables. Woodpecker will try to read the value directly from this file. Keep in mind that when the original environment variable gets specified at the same time it will override the value read from the file.

This is true for all but DATABASE_DATASOURCE_FILE

My referred solution was to stay with the preference rules stated above but use the least precedence way of setting the datasource in the docker image.

Anyway, thanks for taking the issue seriously.

@anbraten anbraten changed the title Woodpecker preferred datasource file over database string Woodpecker prefer datasource file content over env var Feb 21, 2024
@anbraten anbraten changed the title Woodpecker prefer datasource file content over env var Correctly prefer datasource env var over file content Feb 21, 2024
@6543
Copy link
Member Author

6543 commented Feb 21, 2024

The cli lib we use is working on a v3 ... i have a look if there it is posible to specify the prio. If not atm you can suggest that :)

@smainz
Copy link
Contributor

smainz commented Feb 21, 2024

If you use a file for overriding the default value instead of setting the env var directly, it is the least precedence way.

So the only problem is how we set up the Dockerfile. This makes a setting, which is meant to be overridden a de facto hard coded value thus eliminating the flexibility urfave/cli provides.

@6543 6543 closed this Feb 26, 2024
@6543 6543 deleted the WOODPECKER_DATABASE_DATASOURCE_FILE branch February 26, 2024 18:38
@woodpecker-bot
Copy link
Collaborator

Tearing down https://woodpecker-ci-woodpecker-pr-3419.surge.sh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting WOODPECKER_DATABASE_DATASOURCE_FILE does not work as expected when using docker image
3 participants