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

Make sure entrypoint is chown'ed as well #1201

Closed
wants to merge 1 commit into from
Closed

Make sure entrypoint is chown'ed as well #1201

wants to merge 1 commit into from

Conversation

stavros-k
Copy link

Was looking something unrelated, and noticed this. With the . you only chown the WORKDIR

Proposed Changes

Please check that the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Added yourself to AUTHORS.rst

Other questions

  • Do users need to run some commmands in their local instances due to this PR
    (e.g. database migration)?

Was looking something unrelated, and noticed this.
With the `.` you only chown the `WORKDIR`
@rolandgeider
Copy link
Member

Thanks for the PR.

I'm not sure if there was a reason why we only chown the source folder. Alternatively, couldn't we just update the chmod line above?

@stavros-k
Copy link
Author

The chmod does not do chown actions. Or am I missreading something?

@rolandgeider
Copy link
Member

no, but I meant that we could change the chmod +x line to chmod a+x (unless +x and a+x is already the same, I can never remember)

@stavros-k
Copy link
Author

Aha, well yes a+x will set the execute bits for everyone (user, group, others).
But it will still be owned by root.
Tbh I don't know how it worked so far :P As the wger user isn't root.
I haven't looked deep into to check, so I can't really tell.

But I believe it's best to have it owned by the user it will be executed.
To avoid headaches in the future. With user inconsistencies.

@rolandgeider
Copy link
Member

ahh, I see what you mean. I'll try to do some tests later today and check that everything still works

@stavros-k
Copy link
Author

Cool! I noticed this in our CI (TrueCharts Catalog For TrueNAS Scale)
exec /home/wger/entrypoint.sh: exec format error
And thought this might be it, but this was running like this for months.
And I don't see anything changed. Anyway. If I get some time I might try to dive into it myself too

(Not sure if you can see full logs https://github.com/truecharts/charts/actions/runs/3681327880/jobs/6228221329)

@rolandgeider
Copy link
Member

we had some problems a couple of days ago so that the arm64/amd64 images weren't being built correctly (or only one of them was so that you'd sometimes get the wrong arch), but that is fixed already. As far as I could reproduce it, you'd get the same "format error" when using the wrong image

@stavros-k
Copy link
Author

we had some problems a couple of days ago so that the arm64/amd64 images weren't being built correctly (or only one of them was so that you'd sometimes get the wrong arch), but that is fixed already. As far as I could reproduce it, you'd get the same "format error" when using the wrong image

I see, I'll trigger a CI run to pull latest images

@stavros-k
Copy link
Author

we had some problems a couple of days ago so that the arm64/amd64 images weren't being built correctly (or only one of them was so that you'd sometimes get the wrong arch), but that is fixed already. As far as I could reproduce it, you'd get the same "format error" when using the wrong image

I see, I'll trigger a CI run to pull latest images

This fixed it, thanks for the heads up

@stavros-k stavros-k closed this by deleting the head repository Aug 10, 2023
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.

None yet

2 participants