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.

Sign up for GitHub

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

Jump to bottom

Fix permissions for all files given in s2i #59

Merged
merged 1 commit into from
Oct 3, 2019
Merged

Fix permissions for all files given in s2i #59

merged 1 commit into from
Oct 3, 2019

Conversation

phracek
Copy link
Member

@phracek phracek commented Sep 20, 2019 •
edited
Loading

Signed-off-by: Petr "Stone" Hracek phracek@redhat.com

Adds fix-permissions right after cp and add -a to cp command.

Reference issue:
sclorg/container-common-scripts#119

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

5 similar comments
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@rhscl-bot
Copy link

Can one of the admins verify this patch?

@rhscl-bot
Copy link

Can one of the admins verify this patch?

@rhscl-bot
Copy link

Can one of the admins verify this patch?

@pkubatrh
Copy link
Member

Please also add some description in the commit message of why this change is needed as recommended by Pavel in a different PR.

@phracek
Copy link
Member Author

phracek commented Sep 26, 2019

The description message was updated.

@pkubatrh
Copy link
Member

Thanks!
[test][test-openshift]

@pkubatrh
Copy link
Member

pkubatrh commented Oct 1, 2019

rhel8 failure because of:

cp: preserving times for '/etc/varnish/.': Operation not permitted

Seems like we should not run fix-permissions with following symlinks here either

@phracek
Copy link
Member Author

phracek commented Oct 1, 2019

The error cp: preserving times for '/etc/varnish/.': Operation not permitted is not caused by fix-permissions but replacing -R with -a. I have tested locally.

@phracek
Copy link
Member Author

phracek commented Oct 1, 2019

The latest commit message fixes error with /etc/varnish/.: Operation not permitted.

@pkubatrh
Copy link
Member

pkubatrh commented Oct 2, 2019

[test][test-openshift]

@pkubatrh
Copy link
Member

pkubatrh commented Oct 2, 2019

Only problem I see now is that any hidden files will not get copied into ${VARNISH_CONFIGURATION_PATH}.
@phracek Can we just drop the -a argument and go back to -R instead? Considering we run fix-permissions on the destination dir right after the copy, it makes no sense to have it if we need to work around other issues it causes.

@phracek
Fix permission for all files in s2i assemble script
38e2ea0 
Adds fix-permissions right after `mv/cp` command so it keeps
permissions. Also `cp` should use `-a` for keeping permissions.

fix-permissions should be used also at the end of assemble script
because of commands like 'cat' could have had wrong permission
or ownership.

Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
@phracek
Copy link
Member Author

phracek commented Oct 2, 2019

The pull request is updated. Now adds only fix-permission. It would be nice, maybe, to add a test for it. But I will add it later on.

@pkubatrh
Copy link
Member

pkubatrh commented Oct 3, 2019

[test][test-openshift]

@pkubatrh
Copy link
Member

pkubatrh commented Oct 3, 2019

lgtm, thanks!

@pkubatrh pkubatrh merged commit 8884578 into sclorg:master Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@phracek @centos-ci @rhscl-bot @pkubatrh