Skip to content

Review feedback from builds#12229

Merged
smarterclayton merged 1 commit intoopenshift:masterfrom
smarterclayton:build_feedback
Dec 13, 2016
Merged

Review feedback from builds#12229
smarterclayton merged 1 commit intoopenshift:masterfrom
smarterclayton:build_feedback

Conversation

@smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Dec 12, 2016

Follow up to #12218

@smarterclayton smarterclayton mentioned this pull request Dec 12, 2016
Merged
stevekuznetsov
stevekuznetsov suggested changes Dec 12, 2016
View reviewed changes
hack/build-images.sh Outdated
function image {
# image-build is wrapped to allow output to be captured
function image-build {
local tag=$1
Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

Tabs to indent

hack/build-images.sh Outdated
# builds an image and tags it two ways - with latest, and with the release tag
function image {
# image-build is wrapped to allow output to be captured
function image-build {
Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

we use parens

function my-name() {

hack/build-images.sh Outdated
fi

local STARTTIME=$(date +%s)
local STARTTIME=$(date +%s)
Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

don't mix expansions with return codes and scoping statements

hack/build-images.sh Outdated
) > "${out}" 2>&1
local rc=$?
set -e
local ENDTIME=$(date +%s); echo "Finished in $(($ENDTIME - $STARTTIME)) seconds"
Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

if you have nothing else trapped on EXIT just use the exit trap framework

Copy link
Contributor Author

@smarterclayton smarterclayton Dec 12, 2016

Choose a reason for hiding this comment

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

More complicated, don't see the value.

Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

At some point I want to turn on that trap by default for everyone, and I don't see a compelling reason for you to make my life harder :)

Copy link
Contributor Author

@smarterclayton smarterclayton Dec 13, 2016

Choose a reason for hiding this comment

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

this is a trap per test case.

hack/build-images.sh Outdated
local tag=$1
local dir=$2
local out
out="$( mktemp )"
Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

under $BASETMPDIR would be better

hack/build-images.sh Outdated
@@ -125,7 +122,7 @@ function image {
( image openshift/origin-docker-registry images/dockerregistry ) &
( image openshift/origin-egress-router images/router/egress ) &
Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

Shouldn't these be build-image?

Also, correctly written, build-image function shouldn't touch global state and there is no need for the subshells

Copy link
Contributor Author

@smarterclayton smarterclayton Dec 12, 2016

Choose a reason for hiding this comment

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

Don't see any reason to overlap them. And no, they should be image because we are capturing output.

Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

I didn't realize you had image AND image-build ... image is not a super clear method name, from name alone I have no idea what differentiates it from image-build.

hack/common.sh
if [[ -n "$( which imagebuilder )" ]]; then
if os::util::find::system_binary 'imagebuilder'; then
if [[ -n "${dockerfile}" ]]; then
eval "imagebuilder -f '${dockerfile}' -t '${tag}' ${options} '${directory}'"
Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

What are possible values in ${options}? Really don't like the eval here

Copy link
Contributor Author

@smarterclayton smarterclayton Dec 12, 2016

Choose a reason for hiding this comment

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

Arbitrary CLI flags.

Copy link
Contributor

@stevekuznetsov stevekuznetsov Dec 12, 2016

Choose a reason for hiding this comment

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

@Miciah, is it possible to support arbitrary flags like this?

@smarterclayton do you expect those flags to contain spaces? In either case, this just smells like a massive anti-pattern here.

Copy link
Contributor

@Miciah Miciah Dec 13, 2016

Choose a reason for hiding this comment

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

@stevekuznetsov, yeah, the user just needs to use normal shell escaping as necessary. For example, something like OS_BUILD_IMAGE_ARGS='-foo=bar\ baz -bar -baz="quux xyzzy"' should work as expected.

@smarterclayton smarterclayton force-pushed the build_feedback branch 3 times, most recently from 39f836c to c0e78c7 Compare December 13, 2016 00:58
@smarterclayton
Copy link
Contributor Author

smarterclayton commented Dec 13, 2016

[test]

@smarterclayton
Review feedback from builds
a70e5f4 
Remove parallelism for now
@smarterclayton
Copy link
Contributor Author

smarterclayton commented Dec 13, 2016

[test]

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 13, 2016

Evaluated for origin test up to a70e5f4

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 13, 2016

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12306/) (Base Commit: a5ae7f1)

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Dec 13, 2016

Going to merge on green to unblock the queue - parallelism causes docker to throw a wobbler on RHEL.

@smarterclayton smarterclayton merged commit f8b5dd6 into openshift:master Dec 13, 2016
@stevekuznetsov
Copy link
Contributor

stevekuznetsov commented Dec 13, 2016

throw a wobbler

nice

@stevekuznetsov stevekuznetsov mentioned this pull request Dec 13, 2016
Closed
stevekuznetsov added a commit to stevekuznetsov/origin that referenced this pull request Dec 13, 2016
@stevekuznetsov
Revert "Merge pull request openshift#12229 from smarterclayton/build_…
d61563b 
…feedback"

This reverts commit f8b5dd6, reversing
changes made to a5ae7f1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Miciah Miciah Miciah left review comments

@stevekuznetsov stevekuznetsov stevekuznetsov requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smarterclayton @openshift-bot @stevekuznetsov @Miciah