Skip to content

Improves error handling in TektonHub and reorder the job in TektonInstallerSet#645

Merged
tekton-robot merged 1 commit intotektoncd:mainfrom
PuneetPunamiya:refactor-th
Mar 3, 2022
Merged

Improves error handling in TektonHub and reorder the job in TektonInstallerSet#645
tekton-robot merged 1 commit intotektoncd:mainfrom
PuneetPunamiya:refactor-th

Conversation

@PuneetPunamiya
Copy link
Member

@PuneetPunamiya PuneetPunamiya commented Mar 2, 2022 •
edited
Loading

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Release Notes

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 2, 2022
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 2, 2022
@tekton-robot
Copy link
Contributor

tekton-robot commented Mar 2, 2022

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/operator/v1alpha1/tektonhub_defaults.go Do not exist 83.3%
pkg/apis/operator/v1alpha1/tektonhub_lifecycle.go Do not exist 39.1%
pkg/apis/operator/v1alpha1/tektonhub_types.go 100.0% 33.3% -66.7
pkg/apis/operator/v1alpha1/tektonhub_validation.go 100.0% 96.2% -3.8
pkg/reconciler/common/releases.go 41.9% 41.3% -0.7
pkg/reconciler/kubernetes/tektoninstallerset/install.go 76.3% 77.2% 0.9

@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 2, 2022
nikhil-thomas
nikhil-thomas reviewed Mar 2, 2022
View reviewed changes
pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go Outdated
installerSet.Status.MarkControllerReady()

// job
err = installer.IsJobCompleted()
Copy link
Member

@nikhil-thomas nikhil-thomas Mar 2, 2022

Choose a reason for hiding this comment

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

Could you add a log here (info nor error) as the result of this check is not going to show up in any of the existing InstallerSet status conditions.

nikhil-thomas
nikhil-thomas reviewed Mar 2, 2022
View reviewed changes
@PuneetPunamiya PuneetPunamiya force-pushed the refactor-th branch 3 times, most recently from 4277686 to 585010e Compare March 2, 2022 16:12
@PuneetPunamiya PuneetPunamiya mentioned this pull request Mar 2, 2022
Merged
3 tasks
nikhil-thomas
nikhil-thomas reviewed Mar 2, 2022
View reviewed changes
pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go Outdated
Comment on lines 172 to 174
job := installer.Manifest.Filter(mf.ByKind("Job")).Resources()[0]
labels := installerSet.GetLabels()
logger.Info("job not ready in installerset, name: %s, created-by: %s, in namespace: %s", installerSet.GetName(), labels[v1alpha1.CreatedByKey], job.GetNamespace())
return err
Copy link
Member

@nikhil-thomas nikhil-thomas Mar 2, 2022

Choose a reason for hiding this comment

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

this will work but let us move this to

operator/pkg/reconciler/kubernetes/tektoninstallerset/install.go

Line 368 in c6c96e1

return fmt.Errorf("Job not successful")

Doing so will make it easy to report on each job in the installerset instead of focusing on Job[0].

Copy link
Member

@nikhil-thomas nikhil-thomas Mar 2, 2022

Choose a reason for hiding this comment

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

on this line (368) we still need to return the error, but add the log right before it.

Copy link
Member Author

@PuneetPunamiya PuneetPunamiya Mar 3, 2022

Choose a reason for hiding this comment

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

I have updated the patch could you please take a look at it once 🙂

@tekton-robot
Copy link
Contributor

tekton-robot commented Mar 3, 2022

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/kubernetes/tektoninstallerset/install.go 77.2% 77.6% 0.3

@PuneetPunamiya
Copy link
Member Author

PuneetPunamiya commented Mar 3, 2022

/test pull-tekton-operator-integration-tests

1 similar comment
@PuneetPunamiya
Copy link
Member Author

PuneetPunamiya commented Mar 3, 2022

/test pull-tekton-operator-integration-tests

@nikhil-thomas
Copy link
Member

nikhil-thomas commented Mar 3, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2022
@nikhil-thomas
Copy link
Member

nikhil-thomas commented Mar 3, 2022

/retest

@PuneetPunamiya
Copy link
Member Author

PuneetPunamiya commented Mar 3, 2022

/test pull-tekton-operator-integration-tests

@PuneetPunamiya
Copy link
Member Author

PuneetPunamiya commented Mar 3, 2022

/retest

@tekton-robot
Copy link
Contributor

tekton-robot commented Mar 3, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2022
@PuneetPunamiya
Improves error handling in TektonHub and reorder the job in TektonIns…
72661b2 
…tallerSet

Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2022
@tekton-robot
Copy link
Contributor

tekton-robot commented Mar 3, 2022

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/kubernetes/tektoninstallerset/install.go 77.2% 77.6% 0.3

@vdemeester
Copy link
Member

vdemeester commented Mar 3, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2022
@tekton-robot tekton-robot merged commit d036b69 into tektoncd:main Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdemeester vdemeester vdemeester approved these changes

@piyush-garg piyush-garg Awaiting requested review from piyush-garg

@pradeepitm12 pradeepitm12 Awaiting requested review from pradeepitm12

+1 more reviewer

@nikhil-thomas nikhil-thomas nikhil-thomas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vdemeester vdemeester

@nikhil-thomas nikhil-thomas

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PuneetPunamiya @tekton-robot @nikhil-thomas @vdemeester