Adding a check for the set bundle max used in the pipeline controller. #1572

williamlfish · 2022-05-23T21:00:16Z

Currently a user does not know when they might have added too many objects until they attempt to run an image. Adding to the bundle cmd will at least give them feedback before attempting to use the bundle.

Changes

Adding a quick check to the bundle to make sure its layers don't go over the max allowed in the pipeline controller. This is really more for devs using the cli, as it can be a poor ux to create a bundle and not know its unusable till attempting to 😅 .
There is a pr to double the current allowed about here so there is a potential for these to be out of sync and not sure if that should be addressed considering bundles are still in alpha.

Also added some tests 😸

Submitter Checklist

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

Includes tests (if functionality changed/added)
Run the code checkers with make check
Regenerate the manpages, docs and go formatting with make generated
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

The cli will error if attempting to create a bundle with more objects than allowed by the pipeline controller.

tekton-robot · 2022-05-23T21:00:31Z

Hi @williamlfish. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vinamra28 · 2022-05-25T03:54:25Z

/ok-to-test

tekton-robot · 2022-05-25T05:20:38Z

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

File	Old Coverage	New Coverage	Delta
pkg/bundle/builder.go	71.9%	70.6%	-1.3

tekton-robot · 2022-05-25T13:23:44Z

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

File	Old Coverage	New Coverage	Delta
pkg/bundle/builder.go	71.9%	81.1%	9.2

vinamra28 · 2022-05-25T14:58:55Z

pkg/bundle/builder_test.go

+func TestTooManyObjects(t *testing.T) {
+	task := v1beta1.Task{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "tekton.dev/v1beta1",
+			Kind:       "Task",
+		},
+		ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+		Spec:       v1beta1.TaskSpec{Description: "foobar"},
+	}
+
+	raw, err := yaml.Marshal(task)
+	if err != nil {
+		t.Error(err)
+		return
+	}
+	// no error for less then max
+	_, err = BuildTektonBundle([]string{string(raw)}, &bytes.Buffer{})


I wonder if we should be breaking this into multiple functions so that it is more readable?
/cc @piyush-garg @pradeepitm12 @vdemeester

I'm happy to do that, however I have a feeling we may toss this pr out 😅

@williamlfish will be better to have one scenario in a test case

tekton-robot · 2022-05-26T16:53:00Z

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

File	Old Coverage	New Coverage	Delta
pkg/bundle/builder.go	71.9%	81.1%	9.2

williamlfish · 2022-05-27T00:34:27Z

/retest

vinamra28 · 2022-05-30T04:55:03Z

/retest

williamlfish · 2022-05-30T21:28:41Z

/retest

williamlfish · 2022-05-30T21:31:26Z

Is there any known issues with the e2e tests? Running locally I can get them to pass, but I personally noticed they can be a little brittle, mostly with the cluster wide resources, and hitting timeouts 😬

piyush-garg · 2022-07-06T04:46:08Z

Is there any known issues with the e2e tests? Running locally I can get them to pass, but I personally noticed they can be a little brittle, mostly with the cluster wide resources, and hitting timeouts grimacing

tests seems to be working fine

vdemeester · 2022-07-07T08:25:08Z

/retest

Currently a user does not know when they might have added to many objects untill they attempt to run an image. Adding to the bundle cmd will at least give them more imediate feedback before attempting to use the bundle.

williamlfish · 2022-07-22T15:33:13Z

/assign @vdemeester

tekton-robot · 2022-07-25T14:39:52Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdemeester]

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

piyush-garg · 2022-08-01T09:57:17Z

/lgtm

piyush-garg · 2022-08-01T11:29:42Z

/retest

tekton-robot · 2022-08-01T11:45:20Z

@williamlfish: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-tekton-cli-integration-tests	`d0aa6ce`	link	true	`/test pull-tekton-cli-integration-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

tekton-robot requested review from chmouel and hrishin May 23, 2022 21:00

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 23, 2022

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 25, 2022

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 25, 2022

vinamra28 reviewed May 25, 2022

View reviewed changes

tekton-robot requested review from piyush-garg, pradeepitm12 and vdemeester May 25, 2022 14:58

williamlfish force-pushed the bundle/bundle-max branch from 7eaa3b2 to d0aa6ce Compare July 22, 2022 15:29

tekton-robot assigned vdemeester Jul 22, 2022

vdemeester approved these changes Jul 25, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2022

tekton-robot assigned piyush-garg Aug 1, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2022

tekton-robot merged commit 417a52f into tektoncd:main Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a check for the set bundle max used in the pipeline controller. #1572

Adding a check for the set bundle max used in the pipeline controller. #1572

williamlfish commented May 23, 2022 •

edited

Loading

tekton-robot commented May 23, 2022

vinamra28 commented May 25, 2022

tekton-robot commented May 25, 2022

tekton-robot commented May 25, 2022

vinamra28 May 25, 2022

williamlfish May 31, 2022

piyush-garg Jul 20, 2022

tekton-robot commented May 26, 2022

williamlfish commented May 27, 2022

vinamra28 commented May 30, 2022

williamlfish commented May 30, 2022

williamlfish commented May 30, 2022

piyush-garg commented Jul 6, 2022

vdemeester commented Jul 7, 2022

williamlfish commented Jul 22, 2022

tekton-robot commented Jul 25, 2022

piyush-garg commented Aug 1, 2022

piyush-garg commented Aug 1, 2022

tekton-robot commented Aug 1, 2022

Adding a check for the set bundle max used in the pipeline controller. #1572

Adding a check for the set bundle max used in the pipeline controller. #1572

Conversation

williamlfish commented May 23, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 23, 2022

vinamra28 commented May 25, 2022

tekton-robot commented May 25, 2022

tekton-robot commented May 25, 2022

vinamra28 May 25, 2022

Choose a reason for hiding this comment

williamlfish May 31, 2022

Choose a reason for hiding this comment

piyush-garg Jul 20, 2022

Choose a reason for hiding this comment

tekton-robot commented May 26, 2022

williamlfish commented May 27, 2022

vinamra28 commented May 30, 2022

williamlfish commented May 30, 2022

williamlfish commented May 30, 2022

piyush-garg commented Jul 6, 2022

vdemeester commented Jul 7, 2022

williamlfish commented Jul 22, 2022

tekton-robot commented Jul 25, 2022

piyush-garg commented Aug 1, 2022

piyush-garg commented Aug 1, 2022

tekton-robot commented Aug 1, 2022

williamlfish commented May 23, 2022 •

edited

Loading