Skip to content

Fix release 0.1 tests#2166

Closed
bbrowning wants to merge 23 commits intoknative:release-0.1from
bbrowning:fix-release-0.1-tests
Closed

Fix release 0.1 tests#2166
bbrowning wants to merge 23 commits intoknative:release-0.1from
bbrowning:fix-release-0.1-tests

Conversation

@bbrowning
Copy link
Copy Markdown
Contributor

Fixes #2165

Backport enough changes to scripts in test/ and hack/ to hopefully get integration tests running again in the release-0.1 branch.

I'll be amazed if this works on the first attempt, but it's close enough to let CI have at it and see what's broken.

adrcunha and others added 16 commits October 5, 2018 13:16
Partially addresses knative#1575.

`verify-codegen.sh` calls `update-codegen.sh` (which in turns calls `update-deps.sh`) to autogenerate new code, so it can diff the current code in the working tree against the latest autogenerated code (and thus check if everything is up-to-date).

This might leave the working tree in a changed state. This PR undoes all changes to //pkg, //vendor and Gopkg.lock that might be caused by the `update-*.sh` scripts. This way, when running `verify-codegen.sh`, it's guaranteed that your working tree is untouched when the script finishes, as expected.

Bonus: remove redundant environment variables, and use `library.sh` for common/shared stuff, just like the other scripts.
We're consolidating the test infrastructure into a single place, so all repos get the same fixes, updates and new features.

presubmit-tests.sh helper is implemented by knative/test-infra#12
We're consolidating the test infrastructure into a single place, so all repos get the same fixes, updates and new features.

`e2e-tests.sh` helper was implemented by knative/test-infra#17
* Use release.sh helper from prow-tests image

We're consolidating the test infrastructure into a single place, so all repos get the same fixes, updates and new features.

* Update comments
This pulls in the `dep-collector` tool I have been putting together
and uses it to regenerate `./third_party/VENDOR-LICENSE` whenever
`./hack/update-deps.sh` is run.

This also adds a `dep-collector -check` test to our presubmit testing
which will scan our binaries for "forbidden" licenses, and block PRs
from bringing them into the repo.
…it (knative#1639)

* Use library.sh from prow-tests image, instead of yet another copy of it

We're consolidating the test infrastructure into a single place, so all repos get the same fixes, updates and new features.

`library.sh` in prow-tests image was implemented by knative/test-infra#4

* Use start_latest_knative_serving() instead of manually installing
We're consolidating the test infrastructure into a single place, so all repos get the same fixes, updates and new features.

dep-collector was added to prow-tests image in knative/test-infra#22
…e#1654)

Doing clearly separates the components and avoids confusion with the other releases that are build on the same day.
* shared scripts from test-infra live in //vendor/github.com/knative/test-infra/scripts;
* update `update-deps.sh` to keep only the `scripts` folder;
* all bash scripts were updated to use the vendored scripts;
Also use `check_licenses()` from `library.sh`, this will transparently handle the installation of `dep-collector` (if necessary).
Major changes:
* fixes to e2e-tests
* better parsing of e2e tests
Unflake E2E tests due to teardown failures (knative/test-infra#158)
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 5, 2018
@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@bbrowning
Copy link
Copy Markdown
Contributor Author

This incorporates the change from #1922 because without it we have a deadlock scenario where neither PR can pass integration tests without the other. I didn't want to drop @simingweng as the author of that PR, but it does mean we now have multi-author PR that will need some manual intervention to merge. That is, of course, if the tests pass this time.

@bbrowning
Copy link
Copy Markdown
Contributor Author

Well, the good news is the integration tests actually ran! The bad news is BlueGreenRoute and AutoscaleUpDown are flaking, since I didn't backport their fixes. Those changes are more invasive so I don't know if we want to backport those to this branch or just workaround the flakes.

@bbrowning
Copy link
Copy Markdown
Contributor Author

/test pull-knative-serving-integration-tests

adrcunha and others added 5 commits October 5, 2018 16:33
…#1587)

`Error` doesn't contain this useful debugging information.

Fixes knative#1530.
This is a manual cherry-pick of the relevant parts of
56865b8 in an effort to get the
release-1.0 branch tests working again.
Manual cherry-pick of part of 5f88e5e
from master
@bbrowning
Copy link
Copy Markdown
Contributor Author

That took more iterations than I anticipated, but tests are now passing on the release-0.1 branch. The PR looks big, but it only backports changes to Gopkg.toml/lock, scripts in hack/ and test/, and vendored dependencies.

/assign @jessiezcc
/assign @srinivashegde86

@jessiezcc
Copy link
Copy Markdown
Contributor

great to see all tests are passing!
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2018
@jessiezcc
Copy link
Copy Markdown
Contributor

/approve

@jessiezcc
Copy link
Copy Markdown
Contributor

jessiezcc commented Oct 6, 2018

@tcnghia , @mattmoor, need ur approval as well. This covers istio version update too

@vdemeester
Copy link
Copy Markdown
Contributor

what to do about the CLA though ? 🤔

@jessiezcc
Copy link
Copy Markdown
Contributor

@vdemeester, CLAbot has known issue for co-author PR, tracked by https://github.com/knative/serving/issues/1439. We will need to manually merge it.

@bbrowning
Copy link
Copy Markdown
Contributor Author

I can redo this PR without the statsd change, but then it will still be blocked and need manual merging because the integration tests won't actually pass. So, better to have the tests pass and have confidence in the manual merge, in my opinion.

@mattmoor
Copy link
Copy Markdown
Member

mattmoor commented Oct 8, 2018

/approve

/assign @mdemirhan @ZhiminXiang
... since @tcnghia is OOTO this week, and I'd like a second set of eyes on the Networking bits.

Thanks for doing this, @bbrowning.

@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bbrowning, jessiezcc, mattmoor

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2018
@bbrowning
Copy link
Copy Markdown
Contributor Author

The networking change is from #1922. This obsoletes that PR once this gets merged, but I'm not sure if GitHub will pick up on that or if we'll have to specifically close that one.

@adrcunha
Copy link
Copy Markdown
Contributor

/hold

This is a lot of stuff. I was OOO for the past 4 days, let me understand what happened and what broke so we can both (1) have a plan so it doesn't happen again and (2) hopefully we don't have to backport all the entire test-infra stuff to 0.1.

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2018
@mdemirhan
Copy link
Copy Markdown
Contributor

/lgtm
for networking bits.

@bbrowning
Copy link
Copy Markdown
Contributor Author

This is a lot. An alternative I thought about yesterday that we may want to consider is instead branch test-infra from the prior version vendored in the release-0.1 branch in serving. And then just backport the necessary prow config changes to that test-infra branch. That would probably be a much smaller delta of overall changes, at the cost of having a new branch of test-infra.

@adrcunha
Copy link
Copy Markdown
Contributor

#2208 fixes the test infrastructure, without touching anything else (tests themselves or the release stuff).

@bbrowning
Copy link
Copy Markdown
Contributor Author

Closing in favor of #2208. This PR took the approach of getting the release-0.1 branch on a current test-infra and 2208 takes the approach of just porting the needed changes from the current test-infra directly into serving's repo. The latter results in a much smaller set of required changes required in this branch.

@bbrowning bbrowning closed this Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.