disambiguate directory and branch names via -- #14103
Conversation
|
[test] |
|
[test] |
|
not sure why it reported unstable but the new test is failing: https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_check/1830/ |
|
The test is very verbose and there's a fair amount of code copying from pkg/build/builder/source_test.go. The functions gitStatus and createBranch actually appear to be unused. I'd almost be tempted to condense all the exec.* preamble calls into a single call of something like: cmd := exec.Command("bash", "-e", "-c", `
echo test >initial-file
git init
git config user.email [email protected]
git config user.name 'Me Myself'
git add initial-file
git commit -a -m 'repo create'
# etc, etc.
`)
cmd.Path = ??
if err := cmd.Run(); err != nil {
t.Fatal(err)
}I think that would make it simpler and clearer. |
|
On Tue, May 9, 2017 at 4:42 AM Jim Minter ***@***.***> wrote:
The test is very verbose and there's a fair amount of code copying from
pkg/build/builder/source_test.go.
True wrt local repo creation in particular. It seemed trivial enough to me
to not necessitate creating a common unit test package that was leveraged
in both places. Still feel that way, but let me know exactly how much this
bothers you and I'll then see.
The functions gitStatus and createBranch actually appear to be unused.
Ah - meant to delete those. Thanks for the catch. Of course the fact that
it is failing in ci.openshift makes me wonder if I unintentionally checked
in something else.
I'd almost be tempted to condense all the exec.* preamble calls into a
single call of something like:
cmd := exec.Command("bash", "-e", "-c", ` echo test >initial-file git init git config user.email ***@***.*** git config user.name 'Me Myself' git add initial-file git commit -a -m 'repo create' # etc, etc. `)
cmd.Path = ??
if err := cmd.Run(); err != nil {
t.Fatal(err)
}
I think that would make it simpler and clearer.
Assuming that works I like that thanks. It also might facilitate the debug
I was contemplating for the ci.openshift hiccup.
… —
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadCCqqwVAT9wMUEmsOF9nbCe2FbJpks5r4CcJgaJpZM4NUYIM>
.
|
|
Still passing for me locally ... between @jim-minter comments/suggestions and some debug, I'll initiate a new run in a bit. |
gabemontero
force-pushed
the
br-dir-same-name-prob
branch
from
3df2f27 to
8c9a13b
Compare
|
I've pushed an intermediate change. It has a potential resolution to the ci.openshift failure, along with some debug, but only addresses part of @jim-minter 's comments. Assuming it passes, or the debug is at least enlightening, I'll then go back and make further updates per comments. |
|
This is beginning to smell like the check out of pr ref changes, where the git mechanics around checkout are .... curious. Here is the debug from the failure for the test currently running: The above failure also includes a I tried a a) it results in a b) specifying I'm a bit stuck at the moment on how to resolve this. @bparees @jim-minter @csrwng - any ideas ? |
|
@gabemontero have you checked the git level on the ec2 machine? as i recall the stack overflow article indicated the -- support was relatively new. |
|
On Tue, May 9, 2017 at 2:15 PM Ben Parees ***@***.***> wrote:
@gabemontero <https://github.com/gabemontero> have you checked the git
level on the ec2 machine? as i recall the stack overflow article indicated
the -- support was relatively new.
I'll add that check next run thanks @bparees
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadP-uNWU_ZcHmddxWPxm9lVEtn2neks5r4K03gaJpZM4NUYIM>
.
|
|
Though I'm seeing stack overflow and double hyphen from as far back as 2013 ... http://stackoverflow.com/questions/17800994/git-two-dashes-means-with-no-more-options |
|
yeah perhaps not as new as i thought. |
gabemontero
force-pushed
the
br-dir-same-name-prob
branch
from
8c9a13b to
f6b9cec
Compare
|
The git version is quite old: git version 1.8.3.1 Where looking at https://github.com/git/git/commits/master/Documentation/RelNotes/1.8.3.1.txt, its introduction goes as far back as June 2013. It is conceivable the That said, it is somewhat unclear to me whether that is the precise reason for the error. Still somewhat pessimistic though given Given I was at least able to reproduce and verify the fix locally ... I'm beginning to wonder if this is worth the effort and if we should just push the actual runtime change ... :-) |
|
we could also do an extended or integration test (where the clone would actually be getting done by our image w/ a recent git version). |
|
(I do think it's worth having a test for this scenario, i think this is now the second time i've spent a bunch of time investigating why someone's git checkout wasn't doing what we expected before realizing it was a filename conflict w/ their branch name....so i'd really like to be sure we don't regress this :) ) |
|
On Tue, May 9, 2017 at 6:49 PM, Ben Parees ***@***.***> wrote:
we could also do an extended or integration test (where the clone would
actually be getting done by our image w/ a recent git version).
Couldn't tell from the user's comments if there was a reliably available
repo with the same name for the branch / directory ... could you?
If not, I suppose we'd have to create one, no?
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadBvSOFNXggMR2NmpRFYuRU9K7tX4ks5r4O2SgaJpZM4NUYIM>
.
|
yeah i think this would be another case of us making a special branch, perhaps in ruby-hello-world again, to drive the test. Though it's possible we could also drive the test by creating a local git repo w/ an appropriately named branch, and then run "oc start-build --from-repo ." (--from-repo does a binary build using local content, but treating that content as a git repo). |
|
On Tue, May 9, 2017 at 6:57 PM, Ben Parees ***@***.***> wrote:
If not, I suppose we'd have to create one, no?
yeah i think this would be another case of us making a special branch,
perhaps in ruby-hello-world again, to drive the test. Though it's possible
we could also drive the test by creating a local git repo w/ an
appropriately named branch, and then run "oc start-build --from-repo ."
(--from-repo does a binary build using local content, but treating that
content as a git repo).
Unfortunately, with `--from-repo`, s2i is smart enough to use the local
copy and will avoid the `git checkout` code we've updated with the `--`
parameter.
Trace point within
https://github.com/openshift/origin/blob/master/vendor/github.com/openshift/source-to-image/pkg/scm/scm.go#L28-L46
from my testing:
```
2017-05-10T18:39:17.74041738Z I0510 18:39:17.740282 1 scm.go:28]
return from ParseFile file exists true proto specified false use copy true
```
Also, I added a tracepoint in our wrapper to `git checkout` in the builder,
and it did not appear during testing.
For the branch in ruby-hello-world, I'm thinking of naming it "config", as
there is already a config dir, and then remove a file like README.md,
and test for that fact. Thoughts @bparees?
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadDbn49OzJpIMcJmzdQpjIFVpNlLUks5r4O9WgaJpZM4NUYIM>
.
|
sounds fine. |
gabemontero
force-pushed
the
br-dir-same-name-prob
branch
from
f6b9cec to
b07462e
Compare
|
new-app based extended test with our new "config" branch with ruby-hello-world pushed |
|
prior added unit test removed .... was able to verify test broke with fix temporarily reverted |
|
@bparees - same situation as before. The test I added which is passing for me locally is failing up on ec2 / ci.openshift. Could it be that the builder images being constructed up on ec2 / ci.openshift are pulling in the same downlevel I can certainly demo the new test for you on my laptop if a sanity check (of me ;-)) is desired. |
They're going to pull the same level of git as when you build the image yourself. So if it was working in your tests, it should be working in what is being built on ec2. (That said, I do see the git version in our origin-sti-builder is 1.8.3.1) |
|
OK, first, as a sanity check, a gist of the test running locally for me: https://gist.github.com/gabemontero/554da18047a9502f14a60e003ab5da55 Followed by a I'll post an update in a bit with some added debug:
|
|
OK, here is the debug version with the 2 updates I mentioned in my last comment: https://gist.github.com/gabemontero/5509c6906cdec3817e609eae65ef39b1 Of note:
Given we've seen this now with both the unit test and extended test versions of this ... I'm officially at a loss :-( |
|
@gabemontero in the latest extended test run i only see pipeline test failures, i don't see that your new extended test failed. Am I missing something? |
|
I see..it failed in the conformance run. but it passed in the extended test run. that is particularly strange. |
|
On Thu, May 11, 2017 at 5:43 PM Ben Parees ***@***.***> wrote:
@gabemontero <https://github.com/gabemontero> in the latest extended test
run i only see pipeline test failures, i don't see that your new extended
test failed. Am I missing something?
with the new test going in new_app.go it falls in the conformance test
category and shows up under "tests" vs "testextendedonly"
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadN5qdnzfKHrP3ePI6MGxOu3GDOLnks5r44ENgaJpZM4NUYIM>
.
|
it actually runs in both and passed in the extended bucket. |
|
updating the test to run with build loglevel 5 might provide a little more insight, but i'm baffled as to why it would pass in one context and fail in the other. |
|
On Thu, May 11, 2017 at 6:25 PM Ben Parees ***@***.***> wrote:
updating the test to run with build loglevel 5 might provide a little more
insight, but i'm baffled as to why it would pass in one context and fail in
the other.
any chance the sti-builder image is not updated in the failing context?
I could add a level 5 or so trace point in the check out path, rerun with
build trace and see what appears i suppose
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadL5NwFowEfsFrGkldrs0Y54YGAbsks5r44rjgaJpZM4NUYIM>
.
|
hard to imagine and slightly terrifying if true... but i can't claim it's impossible. |
gabemontero
force-pushed
the
br-dir-same-name-prob
branch
from
b07462e to
a10b905
Compare
|
ok just pushed a debug commit that sets BUILD_LOGLEVEL and adds a trace point in repository.go's Checkout.... should hopefully answer at least some of our recent questions |
|
Clayton confirmed none of the images are getting freshly built for the gce conformance tests. Will move the test to a different bucket. |
gabemontero
force-pushed
the
br-dir-same-name-prob
branch
from
a10b905 to
adce895
Compare
|
test case moved ... will report on testing results as they complete. and of course comments on these new changes welcome. |
|
lgtm |
|
Evaluated for origin testextended up to adce895 |
|
Evaluated for origin test up to adce895 |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1400/) (Base Commit: f24a57f) |
|
test error was flake #14161 in conformance install... new test case not executed in that suite |
|
continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/382/) (Base Commit: f24a57f) (Extended Tests: core(builds)) |
|
test extended error was our known jenkins pipeline flake #13984 the new test in this pull passed, as did all the other build extended tests |
|
so only known flakes in the last set of test/testextended ... per #14103 (comment) please post the merge comment , shall the merge comment get posted @bparees ? thanks |
|
@gabemontero sounds good. |
|
Evaluated for origin merge up to adce895 |
|
On Fri, May 12, 2017 at 3:50 PM, Ben Parees ***@***.***> wrote:
@gabemontero <https://github.com/gabemontero> sounds good.
[merge]
thanks @bparees
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadO2Tgku29SLKQzJk1qOW6SeZPh4Dks5r5Lf5gaJpZM4NUYIM>
.
|
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/642/) (Base Commit: af82d3d) (Image: devenv-rhel7_6228) |
Fixes #13933
@openshift/devex ptal