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

extracting Pipeline from SkaffoldConfig #1899

Merged
merged 10 commits into from Apr 3, 2019

Conversation

Projects
None yet
5 participants
@balopat
Copy link
Collaborator

commented Apr 1, 2019

This should be a functionally equivalent change, i.e refactoring, however it does change the generated schema for the skaffold config also wrongly generated due to #1900 :(

Motivation: while introducing RunContext in #1885, it would be beneficial to just share the Pipeline part of the configuration, as:

  • no further functionality depends on the profiles/apiVersion/kind fields anyway
  • the profiles struct is not serializable by gob.encoder as it embeds yamlpatch.Node which does not have exported fields

Update:
#1900 is fixed now, the schema changes are correct.

@balopat balopat requested review from dgageot, nkubala, priyawadhwa and r2d4 as code owners Apr 1, 2019

@googlebot googlebot added the cla: yes label Apr 1, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Apr 1, 2019

Codecov Report

Merging #1899 into master will increase coverage by 0.02%.
The diff coverage is 80.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1899      +/-   ##
==========================================
+ Coverage   49.75%   49.77%   +0.02%     
==========================================
  Files         175      175              
  Lines        8084     8088       +4     
==========================================
+ Hits         4022     4026       +4     
  Misses       3684     3684              
  Partials      378      378
Impacted Files Coverage Δ
pkg/skaffold/runner/dev.go 56.52% <ø> (ø) ⬆️
pkg/skaffold/schema/versions.go 65.85% <ø> (ø) ⬆️
pkg/skaffold/initializer/init.go 21.6% <0%> (ø) ⬆️
cmd/skaffold/app/cmd/runner.go 0% <0%> (ø) ⬆️
pkg/skaffold/schema/v1beta6/upgrade.go 58.97% <100%> (+2.21%) ⬆️
pkg/skaffold/schema/validation/validation.go 100% <100%> (ø) ⬆️
pkg/skaffold/schema/latest/config.go 100% <100%> (ø) ⬆️
pkg/skaffold/runner/runner.go 61.6% <100%> (ø) ⬆️
pkg/skaffold/schema/latest/upgrade.go 100% <100%> (ø) ⬆️
pkg/skaffold/schema/defaults/defaults.go 91.02% <100%> (ø) ⬆️
... and 1 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 0470bad...342a852. Read the comment docs.

Show resolved Hide resolved pkg/skaffold/schema/latest/config.go Outdated
Show resolved Hide resolved pkg/skaffold/schema/latest/config.go Outdated
@tejal29
Copy link
Member

left a comment

Motivation: while introducing RunContext in #1885, it would be beneficial to just share the Pipeline part of the configuration, as:

I understand what was the motivation behind it, but i fail to understand how this is beneficial?
In #1885 are we serializing the Build, Deploy and Test config?

Show resolved Hide resolved pkg/skaffold/runner/runner_test.go Outdated
Show resolved Hide resolved pkg/skaffold/schema/defaults/defaults_test.go Outdated

balopat added some commits Apr 2, 2019

@tejal29

tejal29 approved these changes Apr 3, 2019

Show resolved Hide resolved docs/content/en/docs/references/yaml/main.js Outdated
@balopat

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 3, 2019

I am waiting on the IDE folks to give feedback on this + we might want to merge this into v1beta8 actually as it changes the top-level name in the config.

@balopat

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 3, 2019

rebased + IDE folks gave a thumbs up

@balopat balopat merged commit b1dce64 into GoogleContainerTools:master Apr 3, 2019

4 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kokoro CI build successful.
Details

@tejal29 tejal29 referenced this pull request Apr 12, 2019

Closed

Release v0.27.0 #1951

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.