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

Fixes missing copy command for tdbloader2 and adds procps to image #50

Merged
merged 3 commits into from
Apr 13, 2021

Conversation

kuzeko
Copy link
Collaborator

@kuzeko kuzeko commented Feb 19, 2021

@kuzeko kuzeko requested a review from stain February 19, 2021 16:49
@codextremist
Copy link

Hi, any updates on why this PR hasn't been merged yet?

@kuzeko
Copy link
Collaborator Author

kuzeko commented Mar 17, 2021

@codextremist I have the power to merge, but not sure if should overstep @stain

@alexkreidler
Copy link
Collaborator

Friendly ping @stain. Could we merge this soon?

@kuzeko I would recommend you wait maybe a few days for a response and then merge. If you check the contribution history, a good number of folks have merged PRs and added commits. And I'm sure @stain would rather have the project move forward than wait up and prevent folks from getting bug fixes, etc.

One curious thing is that the CI task seemed to have failed for this PR, but only on one command that greps the output for the word "Started", which hasn't changed since the last few builds. Mysterious...

Thanks to all for your work on this!

@kuzeko
Copy link
Collaborator Author

kuzeko commented Apr 13, 2021

Hi @alexkreidler ,
ok I will merge now.

W.r.t. the CI I am guessing is the fact that the sleep was not "long enough".
If that is the case, that part is better replaced with a call to a for loop that sleeps, queries the system to check if it is up
and then fails completely after some time.

@kuzeko kuzeko merged commit d782555 into stain:master Apr 13, 2021
@kuzeko kuzeko deleted the tdb2 branch April 13, 2021 12:18
@daxid
Copy link

daxid commented May 4, 2021

@alexkreidler @kuzeko I think Docker image is not building with this merge :

https://travis-ci.org/github/stain/jena-docker

This is a major problem leaving the all project basically unusable. Can you look into it please ?

@b2m
Copy link
Collaborator

b2m commented May 5, 2021

W.r.t. the CI I am guessing is the fact that the sleep was not "long enough".

I think it is a little more complicated than that.

Comparing the output of the failed build with previous ones I noticed the difference in the output of docker logs fuseki.

  1. docker logs fuseki
    Shows the admin password part and the server information.
  2. docker logs fuseki | grep --quiet ^admin=
    Is successfull but shows the server info part as output despite getting piped to grep --quiet.
  3. docker logs fuseki | grep --quiet Started
    Is not successfull and shows the server info part as output despite getting piped to grep --quiet.

My hypothesis:

  • the admin password part is written to stdout
  • the server info part is written to stderr
  • grep operates only on stdout
  • therefore the check on ^admin= is successfull whereas the check on Started is not

Possible solution: combine the two channels: docker logs fuseki 2>&1 | grep --quiet Started

@kuzeko
Copy link
Collaborator Author

kuzeko commented May 5, 2021

Good catch @b2m

Here is a pull request to fix it:
#53

@b2m
Copy link
Collaborator

b2m commented May 5, 2021

Here is a pull request to fix it:
#53

Oh, now we can decide whether to merge #52 or #53 =)

@kuzeko
Copy link
Collaborator Author

kuzeko commented May 5, 2021

AH! Nice one.
Mine adds an additional curl call ( I wanted to remove the sleep 8 but then I didn't)
I think we can go with #52

@stain
Copy link
Owner

stain commented May 25, 2021

Thanks @kuzeko, I've also invited @b2m and @alexkreider - feel free to merge without me, my email are particularly lossy at the moment.. :-/

@kuzeko
Copy link
Collaborator Author

kuzeko commented May 25, 2021

Thanks @stain since you are around, do you think you can enable the tagging of the docker containers?
See #31

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.

6 participants