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

propagate profiles across imported configs by default; disable using propagate-profiles flag #5846

Merged
merged 3 commits into from
May 14, 2021

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented May 14, 2021

Fixes: #5707
Related: #5755
Merge before/after: Dependent or prerequisite PRs

Description

Change the behavior of --profile flag for multi-config projects:

  • Profiles specified by --profile are propagated across all imported configs recursively by default
  • This can be disabled by setting --propagate-profiles to false

User facing changes (remove if N/A)

Before:
Users needed to defined this activeProfiles stanza for each and every config dependency in order to activate profiles with the same name (and behavior) across files.

    activeProfiles:                                     
     - name: p1                               
       activatedBy: [p1] 

After:
Users no longer have to specify activeProfiles stanza if they want to activate the same named profile across configs.
Follow-up Work (remove if N/A)

propagate-profiles can be changed from a bool to accept an allow and deny-list of profile names. This can be revisited if there is enough ask for that.

@gsquared94 gsquared94 requested a review from briandealwis May 14, 2021 03:50
@gsquared94 gsquared94 requested a review from a team as a code owner May 14, 2021 03:50
@google-cla google-cla bot added the cla: yes label May 14, 2021
@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #5846 (005868c) into master (d9b397e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5846   +/-   ##
=======================================
  Coverage   70.87%   70.87%           
=======================================
  Files         446      446           
  Lines       16729    16733    +4     
=======================================
+ Hits        11857    11860    +3     
  Misses       3998     3998           
- Partials      874      875    +1     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 89.28% <ø> (ø)
pkg/skaffold/config/options.go 100.00% <ø> (ø)
pkg/skaffold/parser/config.go 79.05% <100.00%> (+0.58%) ⬆️
pkg/skaffold/docker/image.go 78.34% <0.00%> (-1.39%) ⬇️
pkg/skaffold/docker/parse.go 86.19% <0.00%> (-0.96%) ⬇️
pkg/skaffold/util/tar.go 56.00% <0.00%> (+5.33%) ⬆️

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 d9b397e...005868c. Read the comment docs.

Additionally the `activeProfiles` stanza can define the profiles to be activated in the required configs, via:
Profiles specified by the `--profile` flag are also propagated to all configurations imported as dependencies, if they define them. This behavior can be disabled by setting the `--propagate-profiles` flag to `false`.

You can additionally set up more granular and conditional profile activations across dependencies through the `activeProfiles` stanza:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it might helpful to add an example of how --propogate-profiles can be used w/ skaffold /examples directory if a user wants to see it in action easily

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will take up as a followup item.

{path: "doc2/skaffold.yaml", configs: []mockCfg{{name: "cfg20", requiresStanza: ""}, {name: "cfg21", requiresStanza: ""}}},
},
expected: func(base string) []*latestV1.SkaffoldConfig {
return []*latestV1.SkaffoldConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: config_test.go is a bit long atm with 56 test cases in TestGetAllConfigs. From looking at it, this is likely justified as we want the full set of combos for each config option and the test framework is the same for all of them but wanted to point it out as it might be hard for newer users to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree but didn't find any obvious way to simplify it. Do you have any ideas how this test can be better structured?

Copy link
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

@gsquared94 gsquared94 merged commit 64513ee into GoogleContainerTools:master May 14, 2021
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.

Config Imports Should be default pass through the current profile(s)
2 participants