Skip to content

Conversation

mattdelco
Copy link
Contributor

My attempt to fix #3858

I made two attempts to avoid the breakage prior to submit of #3820:

#3820 (comment)
#3820 (comment)

In this change I also remove some space that emacs warns about being suspect (but is probably okay since it's continued from the prior line).

I confirmed that with this change an attempt at "make deb" gets to the point of trying to run fpm (which fails on my system since I don't have it installed, but the point is is compiled istio and got to this step okay).

@mattdelco mattdelco requested a review from a team March 1, 2018 07:30
@mattdelco mattdelco assigned mattdelco and ldemailly and unassigned mattdelco Mar 1, 2018
Copy link
Contributor

@douglas-reid douglas-reid left a comment

Choose a reason for hiding this comment

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

I don't fully understand this fix, but based on your comments, I believe that this should help.

/lgtm

@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: douglas-reid

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@codecov
Copy link

codecov bot commented Mar 1, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #3860    +/-   ##
=======================================
- Coverage      77%     76%   -<1%     
=======================================
  Files         295     295            
  Lines       26463   26863   +400     
=======================================
+ Hits        20227   20403   +176     
- Misses       4989    5159   +170     
- Partials     1247    1301    +54
Impacted Files Coverage Δ
mixer/adapter/stackdriver/stackdriver.go 55% <0%> (-15%) ⬇️
mixer/adapter/rbac/controller.go 55% <0%> (-10%) ⬇️
pilot/pkg/model/context.go 73% <0%> (-2%) ⬇️
mixer/adapter/rbac/rbac.go 10% <0%> (-1%) ⬇️
mixer/adapter/fluentd/fluentd.go 76% <0%> (-1%) ⬇️
mixer/template/sample/template.gen.go 55% <0%> (ø) ⬇️
mixer/adapter/servicecontrol/client.go 0% <0%> (ø) ⬆️
mixer/adapter/memquota/dedup.go 100% <0%> (ø) ⬆️
mixer/adapter/list/ipList.go 100% <0%> (ø) ⬆️
mixer/adapter/kubernetesenv/kubernetesenv.go 69% <0%> (+1%) ⬆️
... and 8 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 5ed46cd...7ffd42b. Read the comment docs.

@istio-merge-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@ldemailly ldemailly merged commit 817cd22 into istio:master Mar 1, 2018
@mattdelco
Copy link
Contributor Author

To be more explicit about the issue, in the Makefile each of the main go executables has two targets. The first target is in the form of the full path to where the executable is created (e.g., "$(ISTIO_OUT)/istioctl") while the second is basically an "phony" alias (e.g., "istioctl") that allows devs to conveniently build a particular executable (e.g., "make istioctl").

The docker and debian targets have dependencies on the full-path target (this helps avoid unnecessary recompiles during rebuilds since make always wants to rebuild "phony" targets), and the one for istioctl was dropped in the earlier PR (hence make's complaint that it doesn't know how to build istioctl).

For the debian image the dependency is set up in tools/deb/istio.mk using the list in ISTIO_DEB_DEPS (and the foreach loop that comes a few lines later) to declare a dependency on each of the go binaries listed in ISTIO_DEB_DEPS (using the full-path target) and also set up a SIDECAR_FILES variable that's used to tell fpm to add each of these listed go binaries to the debian image.

This change adds back the missing target for $(ISTIO_OUT)/istioctl. You might be used to seeing something more of the form:

$(ISTIO_OUT)/istioctl: dependencies
       recipe
istioctl: $(ISTIO_OUT)/istioctl

though a different form that has both targets borrow the same recipe is:

istioctl $(ISTIO_OUT)/istioctl: dependencies
       recipe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

postsubmit failures - istioctl related
6 participants