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

Child params in operators with dependencies failing in pkg resolver #1773

Merged
merged 3 commits into from
Apr 10, 2021

Conversation

alenkacz
Copy link
Contributor

@alenkacz alenkacz commented Feb 12, 2021

What this PR does / why we need it:

Fixes #1772

@alenkacz alenkacz force-pushed the av/dep-params branch 3 times, most recently from d99bc2b to e00806c Compare February 12, 2021 13:32
@alenkacz
Copy link
Contributor Author

The operator test failure is OK, it's because I've cleaned up the operators folder

@alenkacz alenkacz changed the title WIP: Child params in operators with dependencies failing in pkg resolver Child params in operators with dependencies failing in pkg resolver Feb 12, 2021
Signed-off-by: Alena Varkockova <[email protected]>
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.

it would be great to have a newline for end of line of yaml files... otherwise lgtm

@@ -85,7 +85,7 @@ lint: ## Run golangci-lint
ifneq (${GOLANGCI_LINT_VER}, "$(shell golangci-lint version --format short 2>&1)")
./hack/install-golangcilint.sh ${GOLANGCI_LINT_VER}
endif
golangci-lint --timeout 3m run --allow-parallel-runners
golangci-lint --timeout 4m run --allow-parallel-runners
Copy link
Member

Choose a reason for hiding this comment

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

😮 :)

@@ -30,6 +30,12 @@ func (ReferenceVerifier) Verify(pf *packages.Files) verifier.Result {
resources = task.Spec.ResourceTaskSpec.Resources
case engtask.PipeTaskKind:
resources = append(resources, task.Spec.PipeTaskSpec.Pod)
case engtask.KudoOperatorTaskKind:
// param file is being stored in the templates folder, we need to make sure it's in here
// to not treat it as error
Copy link
Member

Choose a reason for hiding this comment

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

👏

if err != nil {
res.AddErrors(fmt.Sprintf("parsing rendered YAML from %s failed: %v", k, err))
// parameter file for KudoOperator task does not have to parse to kubernetes object
if !isParameterFile(k, pf) {
Copy link
Member

Choose a reason for hiding this comment

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

not a big deal... but I'm wondering if we have have a
if isParameterFile() that continues and reduce the indent on the error handling
From a Go perspective, it feels like a lot of nesting... reasonable to keep as well

@asekretenko
Copy link
Member

Hi @alenkacz @kensipe, are you planning to proceed with this PR?

FWICT, upgrade-test failure is also unrelated - this particular flake is observed elsewhere in KUDO CI (I've filed #1783, but most likely it is not KUDO and probably not even KUTTL)

@alenkacz alenkacz merged commit 77ba62f into main Apr 10, 2021
@alenkacz alenkacz deleted the av/dep-params branch April 10, 2021 06:51
@alenkacz
Copy link
Contributor Author

alenkacz commented Apr 10, 2021

@asekretenko merged :) thanks for review @kensipe I missed that you approved, appreciate it! It is such a long time I've implemented this though that I don't remember some of the implementation details :D

alenkacz added a commit that referenced this pull request Apr 21, 2021
…1773)

* Add failing test for depencencies with child params

Signed-off-by: Alena Varkockova <[email protected]>

* Fix verifier handling of child params files

Signed-off-by: Alena Varkockova <[email protected]>

* Increase timeout

Signed-off-by: Alena Varkockova <[email protected]>
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.

kubectl kudo package create fails verification of child KudoOperators parameter files
3 participants