Migrate upgrade tests to use new framework by cardil · Pull Request #4519 · knative/eventing

cardil · 2020-11-12T18:59:48Z

Proposed Changes

Refactors upgrade tests to utilize new upgrade framework

knative-prow-robot · 2020-11-12T18:59:54Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2020-11-12T19:06:41Z

Codecov Report

Merging #4519 (f0905b1) into master (ede5a2a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4519   +/-   ##
=======================================
  Coverage   81.31%   81.31%           
=======================================
  Files         290      290           
  Lines        8157     8157           
=======================================
  Hits         6633     6633           
  Misses       1126     1126           
  Partials      398      398

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 ede5a2a...2c74fa7. Read the comment docs.

cardil · 2020-11-12T19:09:38Z

/test pull-knative-eventing-upgrade-tests

zhongduo · 2020-11-12T20:32:14Z

/assign @zhongduo
Can you remove Draft and WIP when it can be reviewed?

cardil · 2020-11-12T22:38:44Z

@zhongduo sure. After it is completed.

cardil · 2020-11-13T22:15:55Z

/test pull-knative-eventing-upgrade-tests

cardil · 2020-11-16T23:33:47Z

~~Blocked by knative/pkg#1911~~

…de-tests

…de-tests Conflicts fixed: * go.sum * vendor/modules.txt

We need to point ourselves to specific directory as our scripts are location sensitive.

…de-tests

cardil · 2020-11-25T22:16:45Z

This one is ready for review @aliok @zhongduo.

The log output for upgrade tests looks sweet in my opinion. As well as Junit method names! 🎉 🥳

This relies on knative/hack#30 to be merged.

aliok · 2020-11-26T07:12:07Z

 readonly KO_YAML_FLAGS="${KO_YAML_FLAGS} ${KO_FLAGS} --platform=all"

-if [[ -n "${TAG}" ]]; then
+if [[ -n "${TAG:-}" ]]; then


what was the issue here @cardil ? just curious

When using strict bash setting of set -Eeuo pipefail that fails because of -u: fail on unset variable being dereferenced.

Bash could know if variable is unset or empty.

aliok · 2020-11-26T07:16:03Z

/lgtm
/approve
/hold

Looks good to me but I would leave the final approval to @zhongduo

Thanks a lot @cardil

knative-prow-robot · 2020-11-26T07:16:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, cardil

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:

~~OWNERS~~ [aliok]

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

zhongduo

The complexity of this PR are in hack and pkg repo, this is a refactoring to use the new framework. It looks good to me. All my comments are nits, please feel free to ignore them and unhold.

zhongduo · 2020-11-26T16:43:35Z

+			c.Log.Info("Running shell function: ", shellfunc)
+			err := callShellFunction(shellfunc)
+			if err != nil {
+				c.T.Error(err)


Should this be Fatal instead? Or I am guessing the Operation caller will call Fatal when there is an error in installation, right?

It will break operations, yeah.

zhongduo · 2020-11-26T16:47:41Z

+		ops := []string{
+			"install_head",
+			"install_channel_crds",
+			"install_mt_broker",
+			"install_sugar",
+		}


I am a little bit concerned about this as sometimes there may be hacky code in bash script to install it properly, eg waiting to make sure all the pods in the previous step are running before applying next. I am wondering if there is a bash function that does all these installations and we can just call the bash function instead. But for sure we can change it later if such a bash function is required.

There are no such function. Each of those functions has appropriate waits built inside, so it isn't needed.

Current code executes them directly: https://github.com/knative/eventing/pull/4519/files#diff-1c4d417537c9878a5068005f79761334318400204068c4dfd4a7656226bfefe1L61-L64

zhongduo · 2020-11-26T16:48:40Z

+limitations under the License.
+*/
+
+package installation


I wonder if we can just use install instead.

I used installation as I wanted noun, packages should be nouns. install is a verb.

That's why I think installation is better one.

cardil · 2020-11-27T00:17:11Z

/unhold

Unholding as I think I satisfied @zhongduo question. If don't, please comment, and we could follow up.

Migrate upgrade tests to use new framework

305b09f

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 12, 2020

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 12, 2020

knative-prow-robot requested review from yt3liu and zhongduo November 12, 2020 18:59

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Nov 12, 2020

cardil changed the title ~~Migrate upgrade tests to use new framework~~ [WIP] Migrate upgrade tests to use new framework Nov 12, 2020

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 12, 2020

Lint fixes

0aa5b8c

cardil force-pushed the feature/new-upgrade-tests branch from 53b68fb to 0aa5b8c Compare November 12, 2020 19:07

knative-prow-robot assigned zhongduo Nov 12, 2020

cardil added 4 commits November 13, 2020 21:47

Fixing shell outs

a19da86

Removal of unneeded vandor

cd4788a

Lint fixes

5acd72b

Unit test fixes

adfa4ec

cardil mentioned this pull request Nov 16, 2020

Upgrade test hangs if Skip is invoked in continual operation verification phase knative/pkg#1911

Closed

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2020

cardil added 2 commits November 17, 2020 23:31

Merge remote-tracking branch 'upstream/master' into feature/new-upgra…

99c453f

…de-tests

Update deps to include knative/pkg#1912

cd5a8bb

cardil force-pushed the feature/new-upgrade-tests branch from 4f2dd57 to cd5a8bb Compare November 17, 2020 23:03

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2020

cardil changed the title ~~[WIP] Migrate upgrade tests to use new framework~~ Migrate upgrade tests to use new framework Nov 17, 2020

cardil marked this pull request as ready for review November 17, 2020 23:03

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2020

Merge remote-tracking branch 'upstream/master' into feature/new-upgra…

2741bbd

…de-tests Conflicts fixed: * go.sum * vendor/modules.txt

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2020

cardil commented Nov 24, 2020

View reviewed changes

Comment thread go.mod Outdated

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2020

Fixing code style

01dc790

cardil mentioned this pull request Nov 24, 2020

Shell out package knative/hack#30

Merged

cardil added 2 commits November 24, 2020 23:55

Adjusting location of project file

0dd2c3f

We need to point ourselves to specific directory as our scripts are location sensitive.

Bash scripts work with set -Eeuo pipefail

84d6ac4

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2020

Merge remote-tracking branch 'upstream/master' into feature/new-upgra…

21b6248

…de-tests

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2020

cardil marked this pull request as ready for review November 25, 2020 16:54

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2020

Upgrade knative.dev/hack to include knative/hack#30

2c74fa7

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2020

aliok reviewed Nov 26, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2020

knative-prow-robot assigned aliok Nov 26, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2020

zhongduo reviewed Nov 26, 2020

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2020

knative-prow-robot merged commit 6065b38 into knative:master Nov 27, 2020

cardil deleted the feature/new-upgrade-tests branch November 27, 2020 00:58

zhongduo mentioned this pull request Jan 5, 2021

Add zhongduo (Jimmy Lin) as an approver #4691

Merged

Conversation

cardil commented Nov 12, 2020

Proposed Changes

Uh oh!

knative-prow-robot commented Nov 12, 2020

Uh oh!

codecov Bot commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cardil commented Nov 12, 2020

Uh oh!

zhongduo commented Nov 12, 2020

Uh oh!

cardil commented Nov 12, 2020

Uh oh!

cardil commented Nov 13, 2020

Uh oh!

cardil commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cardil commented Nov 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliok commented Nov 26, 2020

Uh oh!

knative-prow-robot commented Nov 26, 2020

Uh oh!

zhongduo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cardil commented Nov 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Nov 12, 2020 •

edited

Loading

cardil commented Nov 16, 2020 •

edited

Loading