Skip to content
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.

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

s2i bump #12237

Merged
merged 2 commits into from
Dec 17, 2016
Merged

s2i bump #12237

merged 2 commits into from
Dec 17, 2016

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Dec 12, 2016

@bparees
Copy link
Contributor Author

bparees commented Dec 12, 2016

[testextended][extended:core(builds|image_ecosystem)]

@bparees
Copy link
Contributor Author

bparees commented Dec 12, 2016

[test]

@bparees
Copy link
Contributor Author

bparees commented Dec 14, 2016

@jim-minter assuming this passes tests now, ptal. particularly where i had to change to use CreateTarStream instead of StreamFileAsTarWithCallback

t := stitar.New()
err := t.StreamFileAsTarWithCallback(src, filepath.Base(dest), uploader, nullWalkFunc, false)
t := s2itar.New(s2iutil.NewFileSystem())
err := t.CreateTarStream(src, false, uploader)
Copy link
Contributor

Choose a reason for hiding this comment

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

	err := t.CreateTarStream(src, false, uploader)

should read:

	tarWriter := s2itar.RenameAdapter{Writer: tar.NewWriter(uploader), Old: filepath.Base(src), New: filepath.Base(dest)}

	err := t.CreateTarStreamToTarWriter(src, true, tarWriter, nil)
	if err == nil {
		err = tarWriter.Close()
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only close tarWriter if err is nil?

Copy link
Contributor

@jim-minter jim-minter Dec 14, 2016

Choose a reason for hiding this comment

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

it's the best I could think of - tarWriter.Close() writes the terminating '\0's record which is supposed to indicate to tar that its stream isn't erroneously truncated.

Copy link
Contributor

Choose a reason for hiding this comment

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

that code is a copy/paste of the equivalent that I implemented in s2i

@jim-minter
Copy link
Contributor

@bparees bar the above change, looks fine to me.

@bparees
Copy link
Contributor Author

bparees commented Dec 14, 2016

@jim-minter it seems plausible the latest s2i changes broke extended builds in origin... (see latest extended test failures)

@bparees
Copy link
Contributor Author

bparees commented Dec 14, 2016

2016-12-14T15:47:03.563740000Z I1214 15:47:03.562961       1 postexecutorstep.go:322] Uploading directory "/tmp/s2i-build213899081/upload/runtimeArtifacts" -> "/opt/jboss"
2016-12-14T15:47:03.564150000Z I1214 15:47:03.562992       1 docker.go:390] Uploading "/tmp/s2i-build213899081/upload/runtimeArtifacts" to "/opt" ...
2016-12-14T15:47:03.564468000Z I1214 15:47:03.563121       1 tar.go:199] Adding "/tmp/s2i-build213899081/upload/runtimeArtifacts" to tar ...
2016-12-14T15:47:03.564871000Z I1214 15:47:03.563441       1 tar.go:285] Adding to tar: /tmp/s2i-build213899081/upload/runtimeArtifacts/hello.jar as runtimeArtifacts/hello.jar
2016-12-14T15:47:03.565371000Z I1214 15:47:03.563711       1 tar.go:285] Adding to tar: /tmp/s2i-build213899081/upload/runtimeArtifacts/scripts/assemble-runtime as runtimeArtifacts/scripts/assemble-runtime
2016-12-14T15:47:03.565577000Z I1214 15:47:03.563926       1 tar.go:285] Adding to tar: /tmp/s2i-build213899081/upload/runtimeArtifacts/scripts/run as runtimeArtifacts/scripts/run
2016-12-14T15:47:03.603253000Z I1214 15:47:03.602678       1 postexecutorstep.go:328] Uploading file "/tmp/s2i-upload-done050185315" -> "/tmp/s2i-upload-done050185315"
2016-12-14T15:47:03.603597000Z I1214 15:47:03.602711       1 docker.go:390] Uploading "/tmp/s2i-upload-done050185315" to "/tmp" ...
2016-12-14T15:47:03.603925000Z I1214 15:47:03.602844       1 tar.go:199] Adding "/tmp/s2i-upload-done050185315" to tar ...
2016-12-14T15:47:03.604548000Z I1214 15:47:03.602893       1 tar.go:285] Adding to tar: /tmp/s2i-upload-done050185315 as s2i-upload-done050185315
2016-12-14T15:47:04.066532000Z I1214 15:47:04.065901       1 postexecutorstep.go:340] /bin/sh: /opt/jboss/scripts/assemble-runtime: Permission denied

seems like the permissions aren't getting set properly on the assemble-runtime script... investigating.

@bparees
Copy link
Contributor Author

bparees commented Dec 14, 2016

@php-coder it looks like the scripts in your test repo are not marked executable:
https://github.com/php-coder/java-maven-hello-world/tree/s2i-assemble-and-assemble-runtime/.s2i/bin

I guess this used to work, but would you expect it to work?

@bparees
Copy link
Contributor Author

bparees commented Dec 14, 2016

prereq PRs (I think):
openshift/test-maven-app#2
openshift/test-maven-app#1

@php-coder
Copy link
Contributor

@bparees Thank you, Ben! It works before but now I don't know how :-/

@bparees
Copy link
Contributor Author

bparees commented Dec 15, 2016

I think our Tar logic may have been updating the permissions.

@bparees
Copy link
Contributor Author

bparees commented Dec 15, 2016

still failing :(

postexecutorstep.go:340] /bin/sh: /opt/jboss/scripts/assemble-runtime: Permission denied

I assume it has to do w/ this change in s2i:
openshift/source-to-image@c0eb0e3#diff-6b4b95c3fb2a650ac8b9a0c5eabdb8b3L129

And that we are now not going through the same chmod path.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2016
@jim-minter
Copy link
Contributor

My bad. I'm hoping that openshift/source-to-image#662 will fix this, but I'm still testing it here.

@jim-minter
Copy link
Contributor

Confirmed, with this PR + openshift/source-to-image#662, the extended case 'should use assemble-runtime script from URL' now works for me.

@php-coder I don't mind either way but I would be tempted to revert openshift/test-maven-app#1 and openshift/test-maven-app#2, FWIW.

@php-coder
Copy link
Contributor

@jim-minter You want to revert them just to make sure that your fix is working?

@bparees
Copy link
Contributor Author

bparees commented Dec 15, 2016

@php-coder shouldn't be necessary, since we're still broken right now (pending @jim-minter's changes), so we should already know if his changes fix it.

@php-coder
Copy link
Contributor

php-coder commented Dec 15, 2016

@bparees Ok. Just in case I invited you and @jim-minter to be collaborators in this repo, so you will be able to do any changes that may be required. I don't want to block you.

@bparees
Copy link
Contributor Author

bparees commented Dec 15, 2016

@php-coder thanks!

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2016
@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to f99d782

@bparees
Copy link
Contributor Author

bparees commented Dec 16, 2016

flake #10663
[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to f99d782

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12465/) (Base Commit: 7061e0b)

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/924/) (Base Commit: 7061e0b) (Extended Tests: core(builds|image_ecosystem))

@bparees
Copy link
Contributor Author

bparees commented Dec 16, 2016 via email

@bparees
Copy link
Contributor Author

bparees commented Dec 17, 2016

flake #9886
[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to f99d782

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 17, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12474/) (Base Commit: 48b0a74) (Image: devenv-rhel7_5563)

@openshift-bot openshift-bot merged commit 3b52b42 into openshift:master Dec 17, 2016
@bparees bparees deleted the s2i_bump branch December 21, 2016 21:49
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.

4 participants