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

Teach package verifier about pipe tasks. #1402

Merged
merged 1 commit into from
Mar 10, 2020
Merged

Teach package verifier about pipe tasks. #1402

merged 1 commit into from
Mar 10, 2020

Conversation

porridge
Copy link
Member

@porridge porridge commented Mar 9, 2020

What this PR does / why we need it:

Collect keys from all pipetasks and feed that to template rendering in place of
an empty map. This has lots of room for improvement, but at least does not fail
on every single package which uses Pipe tasks.

Fixes #1398

Signed-off-by: Marcin Owsiany mowsiany@D2iQ.com

@kensipe
Copy link
Member

kensipe commented Mar 9, 2020

@porridge as an FYI... the package verifier was intended to ignore new templating features it didn't know about... I'm on board with writing a solution for pipes... but I would like to make sure that errors are not reported for unknown features.. than fix pipe verification... perhaps this does that... I will be looking at the solution shortly.

@kensipe
Copy link
Member

kensipe commented Mar 9, 2020

I agree with @zen-dog comments... also... It is worth the time to understand which commit caused this and why. This package verify works with kudo v0.10.x and fails with v0.11.0. There isn't a feature added that should have challenged this.

IMO there should be 2 distinct issues here:

  1. what broke verification which includes pipes? how did it break and lets resolve that.
  2. Adding pipes specifically as a verification feature. With this, I expect a pipe.ReferenceVerifier added to the verify.go.

With step 2 above... I would expect an error for all pipe defs which are incomplete... pipes are more difficult to know if the var is used or not... which is unfortunate... but we should ensure that the pipe task is formed well.

Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
@porridge
Copy link
Member Author

@kensipe this PR addresses the first point, I just filed #1407 about the second one.
@zen-dog I applied your suggestions.
PTAL

Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

Lgtm 🚢

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

woot

@kensipe kensipe merged commit 53e3510 into master Mar 10, 2020
@kensipe kensipe added this to Done in KUDO Global via automation Mar 10, 2020
@kensipe kensipe deleted the verifier-pipes branch March 10, 2020 18:38
runyontr pushed a commit that referenced this pull request Mar 11, 2020
Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Signed-off-by: Thomas Runyon <runyontr@gmail.com>
zen-dog pushed a commit that referenced this pull request Mar 11, 2020
Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
KUDO Global
  
Done
Development

Successfully merging this pull request may close these issues.

Package Verify Pipe Bug
3 participants