Skip to content
This repository has been archived by the owner on Jun 19, 2022. It is now read-only.

update all deps #1257

Merged
merged 4 commits into from
Jun 10, 2020
Merged

update all deps #1257

merged 4 commits into from
Jun 10, 2020

Conversation

capri-xiyue
Copy link
Contributor

@capri-xiyue capri-xiyue commented Jun 10, 2020 •
edited by nachocano
Loading

Fixes #

Proposed Changes

  • update all deps

Release Note

NONE

Docs

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: capri-xiyue

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:

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

@googlebot googlebot added the cla: yes (override cla status due to multiple authors bug) label Jun 10, 2020
capri-xiyue
capri-xiyue commented Jun 10, 2020
View reviewed changes
go.mod Outdated
@@ -75,3 +79,5 @@ replace gopkg.in/yaml.v2 => gopkg.in/yaml.v2 v2.2.2
replace honnef.co/go/tools => honnef.co/go/tools v0.0.1-2019.2.3

replace sigs.k8s.io/yaml => sigs.k8s.io/yaml v1.1.0

replace knative.dev/test-infra => knative.dev/test-infra v0.0.0-20200610004422-8b4a5283a123
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pin to specific knative.dev/test-infra to fix the error when running ./hack/update-deps.sh --upgrade
The error logs is as follows:

github.com/google/knative-gcp/hack imports knative.dev/pkg/hack imports knative.dev/test-infra/scripts: ambiguous import: found package knative.dev/test-infra/scripts in multiple modules: knative.dev/test-infra v0.0.0-20200608183332-3442736aebd0 (/usr/local/google/home/xiyue/go/pkg/mod/knative.dev/test-infra@v0.0.0-20200608183332-3442736aebd0/scripts) knative.dev/test-infra/scripts v0.0.0-00010101000000-000000000000 (/usr/local/google/home/xiyue/go/pkg/mod/knative.dev/test-infra/scripts@v0.0.0-20200

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether it's the right way to handle this(I searched Google and that's what stackoverflow suggested)

Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment saying that you are getting an ambiguous import problem otherwise?... in package knative.dev/test-infra/scripts

@chizhg any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the ambiguous import is because knative serving also specifies specific version of knative.dev/test-infra

Copy link
Member

Choose a reason for hiding this comment

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

@chizhg fixed it here: knative/test-infra#2185
can you revert this test-infra portion once the other PR gets merged and update deps again?

@capri-xiyue
Copy link
Contributor Author

capri-xiyue commented Jun 10, 2020 •
edited
Loading

fyi @nachocano
I updated all the deps including serving(fixed the topic failure because of svc doesn't have isReady anymore) and eventing(fixed the e2e compile failure because of break change of eventing test-infra)

Have to pin to specific knative.dev/test-infra version to fix the ambiguous import error when running ./hack/update-deps.sh --upgrade

@capri-xiyue capri-xiyue changed the title update deps update all deps Jun 10, 2020
@capri-xiyue capri-xiyue mentioned this pull request Jun 10, 2020
Closed
nachocano
nachocano reviewed Jun 10, 2020
View reviewed changes
test/e2e/e2e_test.go Outdated
@@ -136,7 +136,7 @@ func TestBrokerDeadLetterSink(t *testing.T) {
t.Skip("Skipping until https://github.com/google/knative-gcp/issues/485 is fixed.")
cancel := logstream.Start(t)
defer cancel()
e2ehelpers.BrokerDeadLetterSinkTestHelper(t, "MTChannelBasedBroker" /*brokerClass*/, channelTestRunner, lib.DuplicatePubSubSecret)
e2ehelpers.TestBrokerWithConfig(t, "MTChannelBasedBroker" /*brokerClass*/, channelTestRunner, lib.DuplicatePubSubSecret)
Copy link
Member

Choose a reason for hiding this comment

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

good catch not to remove the test!

@nachocano
Copy link
Member

/lgtm
/hold
in case @chizhg has some input,
maybe we can create an issue in test-infra to track this?

@capri-xiyue capri-xiyue requested a review from chizhg June 10, 2020 04:41
@capri-xiyue
Copy link
Contributor Author

/retest

@capri-xiyue
Copy link
Contributor Author

/test pull-google-knative-gcp-build-tests

@nachocano
Copy link
Member

/lgtm

@capri-xiyue
Copy link
Contributor Author

/unhold

@chizhg
Copy link
Member

chizhg commented Jun 10, 2020

/retest

@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/apis/intevents/v1alpha1/topic_lifecycle.go 59.4% 57.6% -1.8
pkg/reconciler/intevents/topic/topic.go 70.2% 69.6% -0.5

@capri-xiyue
Copy link
Contributor Author

/test pull-google-knative-gcp-wi-tests

1 similar comment
@capri-xiyue
Copy link
Contributor Author

/test pull-google-knative-gcp-wi-tests

@knative-test-reporter-robot
Copy link

The following jobs failed:

Test name Triggers Retries
pull-google-knative-gcp-wi-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-google-knative-gcp-wi-tests:

github.com/google/knative-gcp/test/e2e.TestSmokeGCPBroker
github.com/google/knative-gcp/test/e2e.TestGCPBrokerTracing
github.com/google/knative-gcp/test/e2e.TestCloudAuditLogsSourceWithGCPBroker

@capri-xiyue
Copy link
Contributor Author

/retest

@knative-prow-robot knative-prow-robot merged commit 5202391 into google:master Jun 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nachocano nachocano nachocano left review comments

@ericlem ericlem Awaiting requested review from ericlem

@grantr grantr Awaiting requested review from grantr

@chizhg chizhg Awaiting requested review from chizhg

Assignees

@nachocano nachocano

Labels
approved cla: yes (override cla status due to multiple authors bug) lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@capri-xiyue @knative-prow-robot @nachocano @chizhg @knative-metrics-robot @knative-test-reporter-robot @googlebot