Skip to content

Streamline tests #2960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Feb 2, 2018
Merged

Streamline tests #2960

merged 4 commits into from
Feb 2, 2018

Conversation

mattdelco
Copy link
Contributor

@mattdelco mattdelco commented Jan 30, 2018 •
edited
Loading

Some parts of the test seem at best sub-optimal. This change does the following:

  • Removes the now unhandled "-s bookinfo".
  • Additional parameters are now passed to make e2e_all. There's probably a more elegant way to handle E2E_ARGS than what I came up with.
  • I also set ISTIO_DOCKER_HUB so istioctl is built to point to the HUB that's used. I'm not sure if this really helps but it seems like this is the direction that other tests have been headed. I suppose it's getting worthwhile to just have bin/get_workspace_status use HUB instead of the more special-purpose ISTIO_DOCKER_HUB.
  • Replace bin/init.sh with "make depend" (or remove it entirely if the top-level Makefile is being used, as it has appropriate dependencies set).

@mattdelco mattdelco requested a review from a team January 30, 2018 06:04
@mattdelco mattdelco changed the title WIP: streamline tests Streamline tests Jan 30, 2018
@mattdelco mattdelco mentioned this pull request Jan 30, 2018
Merged
chxchx
chxchx reviewed Jan 30, 2018
View reviewed changes
Copy link
Contributor

@chxchx chxchx left a comment

Choose a reason for hiding this comment

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

I guess the e2e smoke has been running all 3 tests for a while instead of just bookinfo and the test infra seems to handle the load well so lets keep it that way.

How does make init differ form make depend? Do you see any changes that need to be made to https://github.com/istio-releases/daily-release/blob/master/prow/daily-release-qualification.sh ?

@istio-merge-robot istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 30, 2018
@istio-merge-robot
Copy link

@mattdelco PR needs rebase

@mattdelco
Copy link
Contributor Author

It would be possible to still run just bookinfo by having the script use a make target of "e2e_bookinfo" instead of "e2e_all". I removed the "-s bookinfo" mostly because it's currently ignored and was causing problems elsewhere as an unrecognized parameter (this might have been when I first tried to place E2E_ARGS="$E2E_ARGS" as an input parameter to make [apparently make isn't so great about handling parameters with space, even when quoted]).

Currently "make depend" is just an alias for "make init", which in turn is mostly an alias for "bin/init.sh". I have a much larger PR where the direct callers of bin/init.sh are complicating things so I'm trying to get rid of the direct callers. I would have just eliminated the call to "bin/init.sh" (like I did elsewhere) but prow/istio-pilot-e2e.sh is calling pilot's Makefile and that file doesn't have all its dependencies set up properly (which my larger PR addresses, in part by deleting pilot's Makefile).

@istio-merge-robot istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 30, 2018
@mattdelco
Copy link
Contributor Author

/test istio-presubmit

@istio-merge-robot
Copy link

/lgtm cancel //PR changed after LGTM, removing LGTM. @chxchx @mattdelco

@mattdelco mattdelco mentioned this pull request Feb 1, 2018
Closed
@mattdelco
Copy link
Contributor Author

/test istio-pilot-e2e

@chxchx
Copy link
Contributor

chxchx commented Feb 1, 2018

/lgtm

@chxchx
Copy link
Contributor

chxchx commented Feb 1, 2018

/assign @sebastienvas
Kind of need this PR to go in before I could enable VM test

@chxchx
Copy link
Contributor

chxchx commented Feb 1, 2018

/test istio-pilot-e2e

@mattdelco mattdelco mentioned this pull request Feb 2, 2018
Merged
@sebastienvas
Copy link
Contributor

/lgtm
/approve

@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chxchx, sebastienvas

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

@sebastienvas sebastienvas merged commit d736054 into istio:master Feb 2, 2018
@chxchx chxchx mentioned this pull request Feb 2, 2018
Merged
@mattdelco mattdelco deleted the master11 branch February 13, 2018 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@chxchx chxchx chxchx left review comments

@sebastienvas sebastienvas sebastienvas approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@chxchx chxchx

@sebastienvas sebastienvas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mattdelco @istio-merge-robot @chxchx @sebastienvas @googlebot @istio-testing