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

Fix CustomTagger docs #4621

Merged
merged 1 commit into from
Aug 4, 2020
Merged

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Aug 3, 2020

fixes #4614

#4588 added a new TagPolicy, CustomTagger, which contains one or more TaggerComponents, each of which itself is a TagPolicy.

since the TagPolicy was inlined, our schema generator was removing it from the generated definition properties - this was causing a nil pointer exception in the javascript.

once this issue was fixed, we have another issue. our templater generates HTML for every schema definition, and nests HTML for every nested schema definition - this means that TagPolicy -> CustomTagger -> TaggerComponent -> TagPolicy causes an infinite loop in the schema generation.

the fix here is to add a new yamlTag, skipTrim, which skips trimming off inlined definitions when set. additionally, our templater will now special case TaggerComponent and yield empty HTML rather than recurse into nested definitions, avoiding the infinite loop.

NOTE: this will fail the schema version check, since this technically updates a released schema version. however, since this is just a yaml tag update and NOT a user facing change, I believe this should be safe. once this is merged to master, all subsequent schema version checks will pass (assuming they don't actually change the released schema).

@nkubala nkubala requested a review from a team as a code owner August 3, 2020 20:38
@nkubala nkubala requested a review from balopat August 3, 2020 20:38
@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #4621 into master will decrease coverage by 0.00%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4621      +/-   ##
==========================================
- Coverage   73.18%   73.17%   -0.01%     
==========================================
  Files         337      337              
  Lines       13261    13272      +11     
==========================================
+ Hits         9705     9712       +7     
- Misses       2947     2950       +3     
- Partials      609      610       +1     
Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100.00% <ø> (ø)
pkg/skaffold/yamltags/tags.go 95.55% <63.63%> (-4.45%) ⬇️

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 6520ff3...6954472. Read the comment docs.

@tejal29 tejal29 added meta/docs-preview docs-modifications runs the docs preview service on the given PR labels Aug 4, 2020
@container-tools-bot
Copy link

Please visit http://35.236.24.125:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Aug 4, 2020
Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

@tejal29 tejal29 merged commit 1e0e9dd into GoogleContainerTools:master Aug 4, 2020
@nkubala nkubala deleted the tag-docs branch August 4, 2020 20:41
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.

Doc: Skaffold.yaml page not rendering
5 participants