Adding -o=name to start-build #9967
Conversation
|
[test] |
| @@ -302,8 +307,7 @@ func (o *StartBuildOptions) Run() error { | |||
| } | |||
| } | |||
|
|
|||
| // TODO: support -o on this command | |||
| fmt.Fprintln(o.Out, newBuild.Name) | |||
| kcmdutil.PrintSuccess(mapper, shortOutput, o.Out, "build", newBuild.Name, "created") | |||
There was a problem hiding this comment.
"started" instead of "created" ?
|
[testextended][extended:core(secrets)] |
|
@bparees Looking into this extended test failure as if it weren't a flake, but please let me know if you recognize this. |
|
@bparees The console output I think key to the failure is: |
| if !t.BuildSuccess { | ||
| t.DumpLogs() | ||
| } | ||
| o.Expect(t.BuildSuccess).To(o.BeTrue()) |
There was a problem hiding this comment.
throwing gomega errors from utility functions is problematic because the test report output will report this line as the point of failure, instead of the actual area in the test. We tend to try to avoid doing this.
There was a problem hiding this comment.
Should now show exact line. gomega.ExpectWithOffset used.
|
@jupierce yeah the key error was this: which is disturbing but not (at least in any way i can see) caused by your changes. hopefully it's related to the docker daemon instability we've been seeing, but let's get a clean extended run before we merge this. |
|
@jupierce please update this PR w/ a before/after example of the start-build output with and without the new flag. |
|
Before PR: After PR: start-build will still send out information to stderr (e.g. "Uploading file "test/extended/testdata/build-secrets/Dockerfile" as binary input for the build ..."), so programmatic use of -o=name should only capture stdout. This is what drove the changes in cli.go to capture stdout/stderr independently. |
|
lgtm pending api review and confirming the extended tests are passing (which may be challenging w/ all the other flake issues we currently have, but i really want to keep extended tests stable and not risk piling more problems into them) |
openshift-bot
added
the
needs-rebase
|
@openshift/api-review |
| if len(o.FromWebhook) > 0 { | ||
| return o.RunStartBuildWebHook() | ||
| } | ||
| if len(o.ListWebhooks) > 0 { | ||
| return o.RunListBuildWebHooks() | ||
| } | ||
|
|
||
| mapper, _ := f.Object(kcmdutil.GetIncludeThirdPartyAPIs(cmd)) |
There was a problem hiding this comment.
why are we including third party APIs here?
There was a problem hiding this comment.
Unnecessary / removed in new commit.
openshift-bot
removed
the
needs-rebase
|
@liggitt anything else for api approval? |
|
cli change looks fine. still seems like there are weird merge commits in the PR |
|
@jupierce lgtm pending commit cleanup |
|
@bparees Extraneous commit removed. |
|
[merge] (hope springs eternal) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7829/) (Image: devenv-rhel7_4810) |
|
re[test] |
1 similar comment
|
re[test] |
|
With flakes cleared now, extended tests found issue in test/cmd/builds.sh . It was reliant on previous output format. Last commit should address. |
openshift-bot
added
the
needs-rebase
|
Flake on deployments |
|
Update to resolve auto-merge conflict. |
openshift-bot
removed
the
needs-rebase
|
re[test] |
|
@bparees All checks passing. |
| @@ -60,6 +61,7 @@ func TestStartBuildWebHook(t *testing.T) { | |||
| Out: buf, | |||
| ClientConfig: cfg, | |||
| FromWebhook: server.URL + "/webhook", | |||
| Mapper: registered.RESTMapper(), | |||
There was a problem hiding this comment.
why wasn't this needed before/why is it needed now?
There was a problem hiding this comment.
Before this PR, mapper was not required. It is now with the introduction of:
kcmdutil.PrintSuccess(o.Mapper, o.ShortOutput, o.Out, "build", newBuild.Name, "started")
If, by "before", you mean interim PR commits, it is because startbuild_test.go was not working but that failure was being masked by the other flakes.
There was a problem hiding this comment.
yeah i just meant before this PR. cool, thanks.
|
[testexended][extended:core(builds)] |
|
[testextended][extended:core(builds)] |
|
@stevekuznetsov no i was trying to run the extended tests with a different ginkgo filter than what was originally specified. unfortunately I typo'd but it looks like @jupierce caught it. |
|
@jupierce those all look like new extended test failures to me, can you investigate to ensure this PR is not introducing them? (or fix it if it is, obviously) |
openshift-bot
added
needs-rebase
|
@bparees All clear. |
| exutil.DumpBuildLogs("docker-build", oc) | ||
| } | ||
| g.By("starting a build") | ||
| br, err := exutil.RunBuildConfigAndWait(oc, "docker-build") |
There was a problem hiding this comment.
I'm a little worried about the proliferation of start build patterns we now have:
- doing oc.Run("start-build") by hand
- exutil.StartBuild
- exutil.RunStartBuild (introduced in this PR)
- exutil.RunBuildConfigAndWait (introduced in this PR)
i like the introduction of the separation of stderr/stdout, but i'd like to see us collapse the existing patterns and get rid of methods where we can.
There was a problem hiding this comment.
If you are ok with the methods introduced in this PR, I will hunt down the last few stragglers.
There was a problem hiding this comment.
I don't love "RunStartBuild" as a name, if we're going to clean this up, i'd suggest keeping the "StartBuild" function name and repurposing it.
And also change RunStartBuildConfigAndWait to "StartBuildConfigAndWait" i think.
(and should StartBuild become StartBuildAndWait ? are there cases where we don't want to wait? How do both of these methods handle cases where I need to expect failure on the start-build operation?)
Also RunStartBuildConfigAndWait can actually take a build argument, no? so it's not well named.
As I think about this more generally, I'm worried about the encapsulation of all this logic. Test cases are still going to need to:
- start a build and not wait (possibly, needed for the serialized build tests I believe)
- start a build and expect failure (so asserting success is not always valid).
So keeping this as a separate set of methods (start a build/buildconfig, wait for a build, check success of a build) seems more logical to me.
There was a problem hiding this comment.
i'd suggest keeping the "StartBuild" function name and repurposing it.
Name simplification sounds good.
and should StartBuild become StartBuildAndWait ? are there cases where we don't want to wait?
Yep. There are few extended tests that specifically test parallel builds.
Also RunStartBuildConfigAndWait can actually take a build argument, no? so it's not well named.
Agreed.
- start a build and not wait (possibly, needed for the serialized build tests I believe)
My RunStartBuild was meant for this -- advanced tests that can't follow the typical pattern / don't want to pass -o=name / can't wait for the result.
- start a build and expect failure (so asserting success is not always valid).
There is a AssertFailure() as well.
https://github.com/jupierce/origin/blob/b9a0173257e99f1767981934793fbc27ba8a1d70/test/extended/builds/docker_quota.go#L42
There was a problem hiding this comment.
There is a AssertFailure() as well.
right, but i can't use that with RunStartBuildConfigAndWait because today that function demands success, it doesn't let me evaluate the outcome, it does it for me.
There was a problem hiding this comment.
discussed on irc, i didn't follow the runstartbuildconfigandwait logic correctly
|
@bparees I believe everything is using StartBuild and StartBuildAndWait now. |
| fmt.Fprintf(g.GinkgoWriter, "\nstart-build output:\n%s\n", out) | ||
| o.Expect(err).To(o.HaveOccurred()) | ||
| g.By("starting the source build with --wait flag and short timeout") | ||
| br, err := exutil.StartBuildAndWait(oc, "source-build", "--wait") |
There was a problem hiding this comment.
doesn't actually need the --wait flag anymore right, since you're handling waiting within the method.
There was a problem hiding this comment.
same comment elsewhere. we should only use the --wait flag if we're explicitly testing --wait behavior somewhere.
There was a problem hiding this comment.
I believe that is true of the example you highlight. It tests whether the error affects the return code of "oc start-buid" when --wait is in use. Searching back through, I found a --wait that was redundant to --follow and a .By(...) string that mentioned --wait, but no longer used it.
Both are now removed, but let me know if you disagree on the presence of others.
There was a problem hiding this comment.
i think it was just using --wait to avoid having to manually check if the build was complete and then check the status. the TC is for testing whether builds get terminated by deadlines, not whether the failure code is properly returned by start-build or not. but we can leave it as is for extra coverage.
|
Evaluated for origin testextended up to cb5824a |
|
Evaluated for origin test up to cb5824a |
|
lgtm, will wait for a final extended test pass |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/425/) (Extended Tests: core(builds), core(secrets)) |
|
[merge] |
|
Evaluated for origin merge up to cb5824a |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7824/) |
| @@ -53,22 +52,11 @@ var _ = g.Describe("[builds][Slow] s2i extended build", func() { | |||
| }) | |||
|
|
|||
| g.By("running the build") | |||
| out, err := oc.Run("start-build").Args("--build-loglevel=5", buildConfigName).Output() | |||
| br, _ := exutil.StartBuildAndWait(oc, buildConfigName, "--build-loglevel=5") | |||
There was a problem hiding this comment.
@jupierce Why we're ignoring error from StartBuildAndWait() here?
There was a problem hiding this comment.
@php-coder I considered it redundant to the AssertSuccess() which follows the line. i.e. if there were a severe error resulting in an err being returned, AssertSuccess would also fail. As part of AssertSuccess, a significant amount of information about the attempted build is output to the Ginkgolog, so we should have everything we need to diagnose.
Fixes #8797