-
Notifications
You must be signed in to change notification settings - Fork 1.7k
'skaffold init' for Kustomize projects generates profiles for each overlay #4349
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
'skaffold init' for Kustomize projects generates profiles for each overlay #4349
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4349 +/- ##
==========================================
+ Coverage 71.75% 71.93% +0.17%
==========================================
Files 325 325
Lines 12613 12708 +95
==========================================
+ Hits 9051 9141 +90
- Misses 2983 2990 +7
+ Partials 579 577 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good except couple of nits.
|
||
// Initializer detects a deployment type and is able to extract image names from it | ||
type Initializer interface { | ||
// deployConfig generates Deploy Config for skaffold configuration. | ||
DeployConfig() latest.DeployConfig | ||
DeployConfig() (latest.DeployConfig, []latest.Profile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: shd we rename DeployConfig
to DeployConfigWithProfiles
since profile is not part of deploy config.
Or else, we could define a new method Profiles
.
Upto you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but in the end decided it was ok since the contract is defined in the interface and is documented. DeployConfigWithProfiles
is technically correct but seemed too verbose to me, especially because it's getting called next to BuildConfig
. I'd vote to leave it alone unless you feel strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative is a separate Profiles()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would also need a big refactor since the profiles get generated basically from excess deploy configuration. i'm not really sure the best way to handle it 🤷
db1a21b
to
6fe50b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few optional things that can be left for the future. They're not functional changes.
|
||
// Initializer detects a deployment type and is able to extract image names from it | ||
type Initializer interface { | ||
// deployConfig generates Deploy Config for skaffold configuration. | ||
DeployConfig() latest.DeployConfig | ||
DeployConfig() (latest.DeployConfig, []latest.Profile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative is a separate Profiles()
.
9dcafbb
to
7eba9ca
Compare
Fixes #4293
this PR changes our support for Kustomize projects in
skaffold init
to generate a profile for each overlay. this will prevent multiple overlays from being applied in a single deploy, which is very likely NOT what users want to do. this is a common setup for projects with shared configuration across multiple environments, where several overlays are specified but only one is applied depending on the environment.with this change, Skaffold will attempt to choose a default overlay to apply on a vanilla run of
skaffold dev
, so that users don't have to pass the explicit profile name on their first run after bootstrapping. the heuristic we'll use is:dev
, use itprod
our directory traversal is ordered, so the generated config will always be deterministic.
users can also opt to provide their desired default overlay using the flag
--default-kustomization
.