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

[Proposal] Transparent skaffold init #4915

Merged

Conversation

MarlonGamez
Copy link
Contributor

Context #1273

Added a design proposal for supporting transparent skaffold init

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #4915 (20c0742) into master (7e5ae4c) will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4915      +/-   ##
==========================================
+ Coverage   72.07%   72.18%   +0.11%     
==========================================
  Files         358      365       +7     
  Lines       12364    12842     +478     
==========================================
+ Hits         8911     9270     +359     
- Misses       2797     2881      +84     
- Partials      656      691      +35     
Impacted Files Coverage Δ
pkg/skaffold/schema/v2beta8/upgrade.go 88.88% <0.00%> (-11.12%) ⬇️
pkg/skaffold/errors/err_map.go 90.56% <0.00%> (-9.44%) ⬇️
pkg/skaffold/deploy/kpt/kpt.go 75.17% <0.00%> (-8.57%) ⬇️
pkg/skaffold/build/gcb/buildpacks.go 92.00% <0.00%> (-8.00%) ⬇️
pkg/skaffold/initializer/build/resolve.go 79.16% <0.00%> (-5.45%) ⬇️
pkg/skaffold/runner/load_images.go 94.73% <0.00%> (-5.27%) ⬇️
pkg/skaffold/build/gcb/kaniko.go 73.33% <0.00%> (-4.45%) ⬇️
...g/skaffold/kubernetes/portforward/entry_manager.go 92.00% <0.00%> (-3.84%) ⬇️
pkg/skaffold/build/local/prune.go 70.11% <0.00%> (-3.38%) ⬇️
pkg/skaffold/docker/parse.go 84.06% <0.00%> (-3.22%) ⬇️
... and 66 more

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 7e5ae4c...5af4796. Read the comment docs.

@MarlonGamez MarlonGamez marked this pull request as ready for review October 15, 2020 18:22
@MarlonGamez MarlonGamez requested a review from a team as a code owner October 15, 2020 18:22
docs/design_proposals/transparent-init.md Show resolved Hide resolved
docs/design_proposals/transparent-init.md Show resolved Hide resolved
docs/design_proposals/transparent-init.md Outdated Show resolved Hide resolved
docs/design_proposals/transparent-init.md Outdated Show resolved Hide resolved
@MarlonGamez
Copy link
Contributor Author

@tejal29 @briandealwis it seems like there are a couple ideas regarding the writing of the skaffold config to disk. I'm personally on the side of either not writing or prompting the user to write.

I see this functionality more as a nice QoL change, rather that something that the user should be made very aware of. Because of this I think it would help to keep it as invisible as possible, and thus have it not leave a trace after running.

Ideally, a user will still run skaffold init to set up their config file for the first time.

I'll try to break down what I think are the pros and cons for each of the options

Not writing:

pros:

  • "it just works"
  • leaves no trace or clutter for the user

cons:

  • the user doesn't know exactly what is happening under the hood

Prompting the user to write:

pros:

  • user will understand what's happening a bit better as they see the used config after run

cons:

  • adds an interactive element to the run, slowing things down potentially

Writing without prompt:

pros:

  • user gets a reusable/modifiable skaffold config after their run

cons:

  • user might be surprised by a file being left behind if they weren't expecting one

@briandealwis
Copy link
Member

@MarlonGamez I think you should book some time to consult with @chairy.

@MarlonGamez
Copy link
Contributor Author

Sorry I never updated this, but I have plans to discuss this with UX this coming Friday. Will hopefully have updates then!

@MarlonGamez
Copy link
Contributor Author

MarlonGamez commented Oct 30, 2020

I had some discussion on the feature with UX. I think that we need to discuss this feature some more with higher bandwidth. Will do so in a meeting

@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 5, 2020
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

@marlon-gamez this looks great, a few outstanding questions but this is about ready i'd say

docs/design_proposals/transparent-init.md Show resolved Hide resolved
docs/design_proposals/transparent-init.md Show resolved Hide resolved
docs/design_proposals/transparent-init.md Show resolved Hide resolved
docs/design_proposals/transparent-init.md Outdated Show resolved Hide resolved
docs/design_proposals/transparent-init.md Outdated Show resolved Hide resolved
docs/design_proposals/transparent-init.md Show resolved Hide resolved
@nkubala
Copy link
Contributor

nkubala commented Nov 17, 2020

CI is failing on an unrelated flake

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.

4 participants