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: allow to template builds.env #2583

Closed
wants to merge 5 commits into from
Closed

feat: allow to template builds.env #2583

wants to merge 5 commits into from

Conversation

caarlos0
Copy link
Member

This PR allows templates in the builds.env section.

This should help improve configs like this one from cosign to something like:

builds:
- id: cosign
  binary: cosign
  main: .
  goos:
    - linux
    - darwin
    - windows
  goarch:
    - amd64
    - arm64
  env:
    - CGO_ENABLED=0
    # option 1:
    - CC_darwin_amd64=o64-clang
    - CCX_darwin_amd64=o64-clang+
    - CC_darwin_arm64=aarch64-apple-darwin20.2-clang
    - CCX_darwin_arm64=aarch64-apple-darwin20.2-clang++
    - CC_windows_amd64=x86_64-w64-mingw32-gc
    - CCX_windows_amd64=x86_64-w64-mingw32-g++
    - 'CC={{ index .Env (print "CC_" .Os "_" .Arch) }}'
    - 'CCX={{ index .Env (print "CCX_" .Os "_" .Arch) }}'
    # option 2:
    - >-
        {{- if eq .Os "darwin" }}
          {{- if eq .Arch "amd64"}}CC=o64-clang{{- end }}
          {{- if eq .Arch "arm64"}}CC=aarch64-apple-darwin20.2-clang{{- end }}
        {{- end }}
        {{- if eq .Os "windows" }}
          {{- if eq .Arch "amd64" }}CC=x86_64-w64-mingw32-gcc{{- end }}
        {{- end }}
    - >-
        {{- if eq .Os "darwin" }}
          {{- if eq .Arch "amd64"}}CXX=o64-clang+{{- end }}
          {{- if eq .Arch "arm64"}}CXX=aarch64-apple-darwin20.2-clang++{{- end }}
        {{- end }}
        {{- if eq .Os "windows" }}
          {{- if eq .Arch "amd64" }}CXX=x86_64-w64-mingw32-g++{{- end }}
        {{- end }}

thoughts?

Signed-off-by: Carlos A Becker <caarlos0@gmail.com>
Signed-off-by: Carlos A Becker <caarlos0@gmail.com>
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 15, 2021
@caarlos0 caarlos0 added this to In progress in Board Oct 15, 2021
@caarlos0 caarlos0 requested a review from a team October 15, 2021 00:52
@caarlos0 caarlos0 self-assigned this Oct 15, 2021
@caarlos0 caarlos0 added the enhancement New feature or request label Oct 15, 2021
@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #2583 (848a718) into master (c6dbd1f) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2583      +/-   ##
==========================================
+ Coverage   84.65%   84.69%   +0.03%     
==========================================
  Files         100      100              
  Lines        7325     7343      +18     
==========================================
+ Hits         6201     6219      +18     
  Misses        929      929              
  Partials      195      195              
Impacted Files Coverage Δ
internal/builders/golang/build.go 92.78% <100.00%> (+0.64%) ⬆️
internal/tmpl/tmpl.go 97.54% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6dbd1f...848a718. Read the comment docs.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

This looks useful in the context you mentioned (as demonstrated by cosign).

I see that the variables are only passed to the build commands, not to hooks - which seems like a good idea in principle IMO, but I'm not sure this intention/behaviour is immediately obvious.

Perhaps that can be addressed with documentation, or a different name, such as build_env? I'm not sure, just thinking out loud... 🤔

@caarlos0
Copy link
Member Author

I see that the variables are only passed to the build commands, not to hooks - which seems like a good idea in principle IMO, but I'm not sure this intention/behaviour is immediately obvious.

noticed that as well, and also not sure why... I think its more likely to be a bug...

@radeksimko
Copy link
Member

I think it depends on what we expect folks to store in these variables, but it was initially intentional as far as I can remember, because hooks are likely to be less trusted as external programs.

Say that there's a vulnerability in that program which runs as post-hook, allowing it to exfiltrate any ENV variables (credentials). Having a more explicit relationship between what variables are passed to each hook reduces the impact of such leak.

This is AFAICT the same reason why GitHub Actions do not have "shared ENV variables" that would enable this "exfiltration by accident" but instead you're forced to type out ${{ secrets.NAME }} in your configs to explicitly say which exact step actually requires a particular secret.

Naturally none of this is concern for the CC_* CCX_* and similar variables in your examples above, but I assume these are not the only way people would use this?

@caarlos0
Copy link
Member Author

hmm, yeah, you're right, that makes sense... leaving as is then... will improve the docs about it later 🙏

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2021
@vercel vercel bot temporarily deployed to Preview October 30, 2021 13:21 Inactive
@caarlos0
Copy link
Member Author

FWIW @radeksimko right now hooks see the build.envs...

@caarlos0
Copy link
Member Author

ok, assuming the env on hooks as well, this is basically wrong. Will close

@caarlos0 caarlos0 closed this Nov 21, 2021
Board automation moved this from In progress to Done Nov 21, 2021
@caarlos0 caarlos0 deleted the tmpl-build-env branch November 21, 2021 15:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2022
@caarlos0 caarlos0 restored the tmpl-build-env branch November 24, 2022 18:59
@caarlos0 caarlos0 deleted the tmpl-build-env branch November 24, 2022 19:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
No open projects
Board
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants