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: env expand contents #721

Merged
merged 4 commits into from
Oct 17, 2023
Merged

feat: env expand contents #721

merged 4 commits into from
Oct 17, 2023

Conversation

caarlos0
Copy link
Member

@caarlos0 caarlos0 commented Oct 4, 2023

closes #719

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 4, 2023
@cloudflare-pages
Copy link

cloudflare-pages bot commented Oct 4, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 33f1b30
Status: ✅  Deploy successful!
Preview URL: https://219da237.nfpm.pages.dev
Branch Preview URL: https://contents-env-expand.nfpm.pages.dev

View logs

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #721 (33f1b30) into main (32c34dd) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #721      +/-   ##
==========================================
+ Coverage   75.08%   75.18%   +0.10%     
==========================================
  Files          10       10              
  Lines        2412     2422      +10     
==========================================
+ Hits         1811     1821      +10     
  Misses        425      425              
  Partials      176      176              
Files Coverage Δ
files/files.go 76.64% <ø> (ø)
nfpm.go 85.65% <100.00%> (+0.59%) ⬆️

@djgilcrease
Copy link
Contributor

Careful with this as we have rejected this in the past because $ is a valid path character. So we need tests for that and need to document that if a $ is in the path it needs to be escaped.

Copy link

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

:shipit:

@AlekSi
Copy link

AlekSi commented Oct 12, 2023

Another option is to opt-in into expanding:

contents:
- src: '${NAME}_${ARCH}'
  dst: /usr/bin/${NAME}
  expand: true

@djgilcrease
Copy link
Contributor

Another option is to opt-in into expanding:

I like this solution, that way it is explicit that the user knows it will expand env vars

@caarlos0
Copy link
Member Author

pushed

@caarlos0 caarlos0 added the enhancement New feature or request label Oct 17, 2023
@caarlos0 caarlos0 self-assigned this Oct 17, 2023
Copy link
Contributor

@djgilcrease djgilcrease left a comment

Choose a reason for hiding this comment

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

LGTM

@caarlos0 caarlos0 merged commit 61d7e9d into main Oct 17, 2023
41 of 42 checks passed
@caarlos0 caarlos0 deleted the contents-env-expand branch October 17, 2023 15:45
@github-actions github-actions bot added this to the v2.34.0 milestone Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support env var expansion for contents.src
3 participants