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

Add yamltags #388

Merged
merged 2 commits into from
Aug 27, 2018
Merged

Add yamltags #388

merged 2 commits into from
Aug 27, 2018

Conversation

dlorenc
Copy link
Contributor

@dlorenc dlorenc commented Apr 13, 2018

This PR adds some new tags we can place on YAML structs that will make management of our config easier.

It adds a 'oneOf' tag, that can be placed on a set of fields and will ensure that only one of them is provided by a user.

It adds a 'required' tag, that can ensure the field has been set by the user.

It adds a 'default' tag, that can be used for strings and ints right now.

Getting this out early for comment. I'll keep cleaning it up and try integrating it into our config parsing next.

@dlorenc
Copy link
Contributor Author

dlorenc commented Apr 15, 2018

Got oneOf working and integrated into all our steps.

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.

Nice! This would definitely be useful outside of skaffold, we should consider moving this code somewhere more central so we can reuse it in other projects.

val.SetInt(i)
case reflect.String:
val.SetString(dt.dv)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that another type could get passed here? Maybe put a default case here that returns an error so it's more clear that other types aren't supported?

@r2d4 r2d4 mentioned this pull request Apr 19, 2018
@r2d4 r2d4 mentioned this pull request Apr 27, 2018
@r2d4 r2d4 added the wip label May 24, 2018
@dgageot dgageot changed the title Add yamltags. [WIP] [WIP] Add yamltags Jun 14, 2018
@dgageot
Copy link
Contributor

dgageot commented Jul 19, 2018

@dlorenc Is this PR still active?

@dlorenc
Copy link
Contributor Author

dlorenc commented Aug 22, 2018

cc @nkubala I rebased this and it should be ready to go if we still want it.

@codecov-io
Copy link

codecov-io commented Aug 22, 2018

Codecov Report

Merging #388 into master will increase coverage by 1.79%.
The diff coverage is 90.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   39.38%   41.17%   +1.79%     
==========================================
  Files          61       62       +1     
  Lines        2613     2703      +90     
==========================================
+ Hits         1029     1113      +84     
- Misses       1471     1473       +2     
- Partials      113      117       +4
Impacted Files Coverage Δ
pkg/skaffold/config/util.go 100% <100%> (ø) ⬆️
pkg/skaffold/yamltags/tags.go 90.58% <90.58%> (ø)
pkg/skaffold/build/local/docker.go 21.05% <0%> (+2%) ⬆️
pkg/skaffold/docker/image.go 59.18% <0%> (+2.19%) ⬆️

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 47865e3...76b368f. Read the comment docs.

@dlorenc dlorenc changed the title [WIP] Add yamltags Add yamltags Aug 27, 2018
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.

one small comment, otherwise LGTM

@@ -10,7 +10,7 @@ build:
gitCommit: {}

# Tag the image with the checksum of the built image (image id).
sha256: {}
# sha256: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to comment this out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this becomes invalid now. You can't define two tag policies with oneOf.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, missed the gitCommit tagger above it.

@nkubala nkubala merged commit cea1e93 into GoogleContainerTools:master Aug 27, 2018
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