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

skipPush -> push #1114

Merged
merged 6 commits into from
Oct 9, 2018
Merged

skipPush -> push #1114

merged 6 commits into from
Oct 9, 2018

Conversation

balopat
Copy link
Contributor

@balopat balopat commented Oct 8, 2018

Fixes #1113.

@balopat balopat mentioned this pull request Oct 8, 2018
11 tasks
@codecov-io
Copy link

codecov-io commented Oct 8, 2018

Codecov Report

Merging #1114 into master will increase coverage by 0.18%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1114      +/-   ##
==========================================
+ Coverage   42.96%   43.14%   +0.18%     
==========================================
  Files          74       77       +3     
  Lines        3403     3537     +134     
==========================================
+ Hits         1462     1526      +64     
- Misses       1801     1858      +57     
- Partials      140      153      +13
Impacted Files Coverage Δ
pkg/skaffold/build/local/types.go 0% <0%> (ø) ⬆️
pkg/skaffold/schema/v1alpha3/upgrade.go 68.75% <88.88%> (ø)
pkg/skaffold/schema/v1alpha3/defaults.go 40.65% <0%> (ø)
pkg/skaffold/schema/v1alpha3/config.go 45.45% <0%> (ø)

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 b809bd5...36c7332. Read the comment docs.

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

@balopat I like that. You should add the code that migrates existing configs though.

@balopat
Copy link
Contributor Author

balopat commented Oct 8, 2018

Thanks for catching that @dgageot! Done.

skaffold.yaml Outdated
@@ -2,7 +2,7 @@ apiVersion: skaffold/v1alpha3
kind: Config
build:
artifacts:
- image: gcr.io/k8s-skaffold/skaffold
- imageName: gcr.io/k8s-skaffold/skaffold
docker:
dockerfile: deploy/skaffold/Dockerfile
Copy link
Contributor

Choose a reason for hiding this comment

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

I think dockerfile should be dockerfilePath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed - I changed it

@dgageot dgageot merged commit ee5e8f1 into GoogleContainerTools:master Oct 9, 2018
@balopat balopat deleted the rename_push branch October 15, 2018 22:19
@@ -77,7 +77,8 @@ build:
# `useBuildkit` can also be set to activate the experimental BuildKit feature.
#
# local:
# skipPush: true
# false by default for local clusters, true for remote clusters
# push: false

Choose a reason for hiding this comment

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

The doc in the previous paragraph still talks about skip push, with inverted boolean value: this is confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants