Remove depend.* targets by nmittler · Pull Request #5148 · istio/istio

nmittler · 2018-04-24T00:57:36Z

We have several depend.* targets that to various things, wrapping
dep ensure. This has caused confusion and has also led to the
breakage of the depend.update target. PR #3678 changed the behavior
of depend.update to remove the --update option when running
dep ensure.

This PR removes all of the depend.* targets and replaces them
with the raw commands in the CircleCI yaml.

costinm

/approve
/lgtm

Thanks !

istio-testing · 2018-04-24T00:58:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm

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~~ [costinm]

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

ldemailly · 2018-04-24T01:04:41Z

Makefile

it's not great to change the behavior of the target that several people got used to use
(e.g make depend.update DEPARGS="--update istio.io/api" )

it also doesn't let you update 1 package anymore does it ?

ldemailly · 2018-04-24T01:06:51Z

btw can you update https://github.com/istio/istio/wiki/Dev-Guide#other-dependencies

ldemailly · 2018-04-24T01:23:00Z

either way just document which is which
(but again dep ensure --update is not recommended - and for instance fails right now on master)

/hold

your responsibility, see my concerns

ldemailly · 2018-04-24T01:42:27Z

In summary I think this target should stay like it was during submodule - at least until it's documented on what it does (it was documented in the vendor faq before) ie that it now does something no one should call/try more than once a release cycle (also anyway that's not the right way to update all; you need what "make depend.update.full" was doing)

But up to you to deal with

Warning: Gopkg.lock is out of sync with Gopkg.toml or the project's imports.
Solving failure: package k8s.io/apimachinery/pkg/conversion/unstructured does not exist within project k8s.io/apimachinery

and similar errors

nmittler · 2018-04-24T15:16:58Z

@costinm @ldemailly It seems that the Makefile targets have become a source of ambiguity in part because they are adding a layer of abstraction over the standard tooling. I propose that we get rid of the depend.update, depend.cleanlock, and depend.update.full targets altogether and change the instructions to simply use dep directly rather than relying on Makefile targets.

WDYT?

ldemailly · 2018-04-24T17:47:35Z

instructions/new FAQ is the biggest missing piece - and also avoiding breaking humans muscle memory

nmittler · 2018-04-24T17:49:57Z

@ldemailly could you comment specifically on: #5148 (comment)?

ldemailly · 2018-04-24T17:53:49Z

that was my specific comment, that whatever you do, document it (first)
I don't mind dropping the targets, it's better to drop them than have them do something wrong
you can maybe add some "ci.depend.*" for the ci (and not humans) to use

istio-testing · 2018-04-30T16:21:08Z

New changes are detected. LGTM label has been removed.

codecov · 2018-04-30T16:38:27Z

Codecov Report

Merging #5148 into master will increase coverage by 1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #5148    +/-   ##
=======================================
+ Coverage      74%     74%    +1%     
=======================================
  Files         323     323            
  Lines       27049   27268   +219     
=======================================
+ Hits        19788   20008   +220     
+ Misses       6483    6472    -11     
- Partials      778     788    +10

Impacted Files	Coverage Δ
mixer/adapter/rbac/controller.go	`39% <0%> (-15%)`	⬇️
mixer/adapter/stackdriver/log/log.go	`69% <0%> (-4%)`	⬇️
mixer/adapter/servicecontrol/checkprocessor.go	`80% <0%> (-2%)`	⬇️
mixer/pkg/attribute/mutableBag.go	`92% <0%> (-1%)`	⬇️
mixer/adapter/kubernetesenv/kubernetesenv.go	`68% <0%> (ø)`	⬇️
mixer/pkg/protobuf/yaml/resolver.go	`100% <0%> (ø)`	⬆️
mixer/adapter/list/ipList.go	`100% <0%> (ø)`	⬆️
mixer/adapter/list/regexList.go	`100% <0%> (ø)`	⬆️
mixer/adapter/stdio/zap.go	`100% <0%> (ø)`	⬆️
mixer/adapter/list/stringList.go	`100% <0%> (ø)`	⬆️
... and 9 more

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 435a996...b395329. Read the comment docs.

nmittler · 2018-04-30T16:43:10Z

@costinm @ldemailly @rshriram PTAL

I've changed this PR to do a couple of things:

Simply remove all of the depend.* targets in favor of running the necessary commands directly in CircleCI.
Running with the latest version of dep.

We have several depend.* targets that to various things, wrapping dep ensure. This has caused confusion and has also led to the breakage of the depend.update target. PR istio#3678 changed the behavior of `depend.update` to remove the `--update` option when running `dep ensure`. This PR removes all of the depend.* targets and replaces them with the raw commands in the CircleCI yaml.

nmittler · 2018-04-30T18:05:33Z

/test e2e-bookInfo-envoyv2-v1alpha3

istio-testing · 2018-04-30T18:19:52Z

@nmittler: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-bookInfoTests-v1alpha3.sh	`b395329`	link	`/test e2e-bookInfo-envoyv2-v1alpha3`

Details

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.

nmittler · 2018-04-30T18:33:34Z

@costinm the failing CI test is unrelated. I've raised #5299. I suspect it's related to the v2 work.

costinm · 2018-04-30T21:44:04Z

Test failure seems unrelated, force merging this to restore testing of update.

We have several depend.* targets that to various things, wrapping dep ensure. This has caused confusion and has also led to the breakage of the depend.update target. PR istio#3678 changed the behavior of `depend.update` to remove the `--update` option when running `dep ensure`. This PR removes all of the depend.* targets and replaces them with the raw commands in the CircleCI yaml.

nmittler requested review from costinm and ldemailly April 24, 2018 00:57

googlebot added the cla: yes label Apr 24, 2018

costinm approved these changes Apr 24, 2018

View reviewed changes

istio-testing assigned costinm Apr 24, 2018

istio-testing added the lgtm label Apr 24, 2018

istio-testing added the approved label Apr 24, 2018

nmittler mentioned this pull request Apr 24, 2018

Adding CI workflow for checking vendor diff #5051

Merged

ldemailly previously requested changes Apr 24, 2018

View reviewed changes

istio-testing added the do-not-merge/hold Block automatic merging of a PR. label Apr 24, 2018

ldemailly changed the title ~~Restoring proper behavior for depend.update target~~ Changing behavior for depend.update target Apr 24, 2018

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 27, 2018

nmittler force-pushed the dep_update branch from 5f0a690 to c71fde2 Compare April 30, 2018 16:21

istio-testing removed lgtm needs-rebase Indicates a PR needs to be rebased before being merged labels Apr 30, 2018

nmittler force-pushed the dep_update branch 3 times, most recently from 8622859 to 874acc4 Compare April 30, 2018 16:37

nmittler force-pushed the dep_update branch from 874acc4 to 9d2066c Compare April 30, 2018 16:40

nmittler changed the title ~~Changing behavior for depend.update target~~ Remove depend.* targets Apr 30, 2018

nmittler force-pushed the dep_update branch from 9d2066c to b395329 Compare April 30, 2018 16:54

costinm merged commit 35e2b90 into istio:master Apr 30, 2018

Conversation

nmittler commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Apr 24, 2018

Uh oh!

ldemailly Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Apr 24, 2018

Uh oh!

ldemailly commented Apr 24, 2018

Uh oh!

ldemailly commented Apr 24, 2018

Uh oh!

nmittler commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly commented Apr 24, 2018

Uh oh!

nmittler commented Apr 24, 2018

Uh oh!

ldemailly commented Apr 24, 2018

Uh oh!

istio-testing commented Apr 30, 2018

Uh oh!

codecov bot commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nmittler commented Apr 30, 2018

Uh oh!

nmittler commented Apr 30, 2018

Uh oh!

istio-testing commented Apr 30, 2018

Uh oh!

nmittler commented Apr 30, 2018

Uh oh!

costinm commented Apr 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nmittler commented Apr 24, 2018 •

edited

Loading

ldemailly Apr 24, 2018 •

edited

Loading

nmittler commented Apr 24, 2018 •

edited

Loading

codecov bot commented Apr 30, 2018 •

edited

Loading