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

Fix chgrp #2834

Merged
merged 1 commit into from
Jan 17, 2022
Merged

Fix chgrp #2834

merged 1 commit into from
Jan 17, 2022

Conversation

UlrichThomasGabor
Copy link
Contributor

This fixes #2832.

The second commit replaces -H with -L (as the comment says) and adds -L to chmod.
This is currently not actually a problem for me, but the current status looks kind of arbitrary to me? Why is writable_mode=chmod using -L but writable_mode=chgrp is using -H for chgrp and nothing for chmod?

@UlrichThomasGabor
Copy link
Contributor Author

I just noticed that GNU chmod does not come with -L (at least the version I have here), so removed it there again...

@antonmedv antonmedv self-requested a review January 6, 2022 09:17
@antonmedv
Copy link
Member

Already fixed.

@antonmedv antonmedv closed this Jan 13, 2022
@UlrichThomasGabor
Copy link
Contributor Author

@antonmedv ehm no, it is not?

run("$sudo chmod g+rwx $dirs");

The $recursive in this line is important and still missing

@antonmedv
Copy link
Member

You are right.

@Schrank Schrank reopened this Jan 13, 2022
@antonmedv
Copy link
Member

Will fix it.

@antonmedv antonmedv closed this Jan 13, 2022
@UlrichThomasGabor
Copy link
Contributor Author

@antonmedv very good!

What is with -L instead of -H here?

run("$sudo chgrp -H $recursive {{http_group}} $dirs");

I think this should be -L as well.

If the mode is chmod it is traversing everything:

run("$sudo chown -L $recursive $httpUser $dirs");

so why should mode chgrp behave differently?

@antonmedv
Copy link
Member

Agree.

@UlrichThomasGabor
Copy link
Contributor Author

@antonmedv You are just going to do it? Should we reopen the ticket? This PR includes that change, so I don't know what I should do more

@antonmedv antonmedv reopened this Jan 17, 2022
@antonmedv
Copy link
Member

Yes, will merge it.

@antonmedv
Copy link
Member

Can you resolve conflicts?

@UlrichThomasGabor
Copy link
Contributor Author

@antonmedv done

@antonmedv antonmedv merged commit b6d646d into deployphp:master Jan 17, 2022
@antonmedv
Copy link
Member

Thanks!

@UlrichThomasGabor UlrichThomasGabor deleted the fix-chgrp branch January 17, 2022 18:39
midweste pushed a commit to midweste/deployer that referenced this pull request Jan 29, 2022
* upstream/master: (35 commits)
  Fix grammar
  [automatic] Update docs with bin/docgen
  Update php.php (deployphp#2916)
  Update getting-started.md (deployphp#2915)
  Fix Magento version detection, fixes deployphp#2905 (deployphp#2906)
  Use -L on chgrp (deployphp#2834)
  Added the Doctrine mapping files validation (with --skip option) as a (possible) deploy task. (deployphp#2901)
  Don't add remote user to setfacl command, if it doesn't exist as an os user (deployphp#2822)
  Update writable.php
  Set version 7.0.0-master
  Docs style
  Use npm ci
  Small fixes
  Remove unused use.
  [automatic] Update docs with bin/docgen
  Refactor deploy:cleanup Command (deployphp#2788)
  Use StrictHostKeyChecking=accept-new by default
  Use StrictHostKeyChecking=accept-new by default
  Move git related options to update_code.php
  composer update
  ...
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.

writable_mode = "chgrp", chgrp is recursive, chmod is not?
3 participants