Skip to content

feat: define skaffold.env file for loading environment variables#8395

Merged
aaron-prindle merged 2 commits intoGoogleContainerTools:mainfrom
gsquared94:fix-1545
Feb 3, 2023
Merged

feat: define skaffold.env file for loading environment variables#8395
aaron-prindle merged 2 commits intoGoogleContainerTools:mainfrom
gsquared94:fix-1545

Conversation

@gsquared94
Copy link
Copy Markdown
Contributor

@gsquared94 gsquared94 commented Feb 1, 2023

Fixes: #1545, #7913

Description

In Skaffold, a skaffold.env file can be defined in the project root directory to specify environment variables that Skaffold will load into the process. This provides a more organized and manageable way of setting environment variables, rather than passing them as command line arguments.

The skaffold.env file should be in the format of KEY=value pairs, with one pair per line. Skaffold will automatically load these variables into the environment before running any commands.

Here is an example skaffold.env file:

ENV_VAR_1=value1
ENV_VAR_2=value2

Setting Skaffold Flags with Environment Variables

In addition to loading environment variables from the skaffold.env file, Skaffold also allows users to set flags using environment variables. To set a flag using an environment variable, use the SKAFFOLD_ prefix and convert the flag name to uppercase.

For example, to set the --cache-artifacts flag to true, the equivalent environment variable would be SKAFFOLD_CACHE_ARTIFACTS=true.

Here is an example usage in the skaffold.env file:

SKAFFOLD_CACHE_ARTIFACTS=true
SKAFFOLD_NAMESPACE=mynamespace

Reasons for not adding it as a skaffold.yaml field

There's no obvious way to incorporate the flags like default-repo and others in the skaffold configuration definition. It only make sense to have a single value per run for these parameters. But right now even as a top level parameter in the skaffold.yaml file, due to multi-module support Skaffold would need to enforce that all the skaffold pipelines define the same value for these flags, and also decide how to resolve conflicts. We avoid that by allowing a single skaffold.env file that can define these flags along with other env variables that will only have a single resolved value despite the number of skaffold modules involved.

@gsquared94 gsquared94 added docs-modifications runs the docs preview service on the given PR docs-modifications-v2 runs the docs v2 preview service on the given PR labels Feb 1, 2023
@container-tools-bot
Copy link
Copy Markdown

Please visit http://34.69.85.167:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications-v2 runs the docs v2 preview service on the given PR label Feb 1, 2023
@container-tools-bot
Copy link
Copy Markdown

Error creating deployment, please see controller logs for details.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Feb 1, 2023
@container-tools-bot
Copy link
Copy Markdown

Error creating deployment, please see controller logs for details.

@container-tools-bot
Copy link
Copy Markdown

Please visit http://34.69.85.167:1313 to view changes to the docs.

@container-tools-bot
Copy link
Copy Markdown

Error creating deployment, please see controller logs for details.

@container-tools-bot
Copy link
Copy Markdown

Error creating deployment docs-controller-deployment-8395, please visit https://storage.googleapis.com/webhook-logs/logs-8395-1675212566638854513 to view logs.

@container-tools-bot
Copy link
Copy Markdown

Error creating deployment, please see controller logs for details.

@container-tools-bot
Copy link
Copy Markdown

Please visit http://35.193.234.120:1313 to view changes to the docs.

@container-tools-bot
Copy link
Copy Markdown

Error creating deployment, please see controller logs for details.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 1, 2023

Codecov Report

Merging #8395 (4acde5a) into main (290280e) will decrease coverage by 4.60%.
The diff coverage is 55.02%.

@@            Coverage Diff             @@
##             main    #8395      +/-   ##
==========================================
- Coverage   70.48%   65.89%   -4.60%     
==========================================
  Files         515      605      +90     
  Lines       23150    29825    +6675     
==========================================
+ Hits        16317    19652    +3335     
- Misses       5776     8699    +2923     
- Partials     1057     1474     +417     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/completion.go 13.04% <0.00%> (-1.25%) ⬇️
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go 65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go 66.66% <ø> (ø)
... and 415 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aaron-prindle
Copy link
Copy Markdown
Contributor

aaron-prindle commented Feb 2, 2023

Can we add 1 integration tests that has a skaffold.env file and a go template that uses an ENV var set in the skaffold.env file in the skaffold.yaml (validating that it was correctly set)? Want to make sure we have a way of verifying this continues to work over time (if we bump the deps or we change our env expansion logic)

@aaron-prindle
Copy link
Copy Markdown
Contributor

Additionally once this is merged, we might need to coordinate w/ parter teams, etc. as some bundle all relevant skaffold files for their pipeline's. We might need to add this to set of files to be tracked

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 3, 2023
Copy link
Copy Markdown
Contributor

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

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

LGTM

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default repository alternative implementation

3 participants