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

CAF::Process supports shell option #255

Open
stdweird opened this issue Nov 9, 2017 · 9 comments
Open

CAF::Process supports shell option #255

stdweird opened this issue Nov 9, 2017 · 9 comments
Assignees

Comments

@stdweird
Copy link
Member

stdweird commented Nov 9, 2017

currently, CAF::Process support the LC::Process shell option (ie assume there's only one element in the command list, and run it as is (incl lack of protection again shell-isms).
we should eitehr document it or remove it. i'm in favour of removing it, but it goes against the 'should allow to shoot yourself in the foot' principle (also known as CAF devs shouldn't pretend to know it all 😉)

@stdweird stdweird added this to the 18.3 milestone Nov 9, 2017
@jrha jrha modified the milestones: 18.3, 18.6 Mar 29, 2018
@jouvin
Copy link
Contributor

jouvin commented Apr 25, 2018

LAL workshop: first step, throw a deprecation warning and at the next workshop, decide to remove the support.

@jrha jrha self-assigned this Jul 2, 2018
jrha added a commit to jrha/CAF that referenced this issue Jul 2, 2018
jrha added a commit to jrha/CAF that referenced this issue Jul 2, 2018
jrha added a commit to jrha/CAF that referenced this issue Jul 2, 2018
@jrha
Copy link
Member

jrha commented Jul 2, 2018

N.B. ncm-filecopy appears to use shell => 1 for restart commands.

@jouvin
Copy link
Contributor

jouvin commented Jul 2, 2018

Exactly, at least ncm-filecopy makes use of it. This seems difficult to remove it, except if we tokenize the command given for the restart property in filecopy. The goal of the deprecation warning was to better identify all the circumstances where a shell is used. I'm afraid that there is a couple of grid components also rely on this feature but I don't remember precisely.

@jrha
Copy link
Member

jrha commented Jul 2, 2018

AFAICT it's only ncm-glitestartup

@jrha jrha modified the milestones: 18.6, 18.9 Jul 3, 2018
jrha added a commit to jrha/CAF that referenced this issue Jul 3, 2018
@jrha
Copy link
Member

jrha commented Jul 3, 2018

As discussed we need a migration strategy for (at least) filecopy that does not place a burden on sites when upgrading.

@jrha
Copy link
Member

jrha commented Oct 5, 2018

No consensus yet, hopefully we can discuss at workshop.

@jrha jrha removed this from the 18.9 milestone Oct 5, 2018
@jrha
Copy link
Member

jrha commented Oct 29, 2018

Discussed at workshop, an option to reduce usage of subshell in filecopy would be to allow a list (or list of lists) of command + arguments for the restart command.

@jrha
Copy link
Member

jrha commented Oct 29, 2018

Another option would be to try and reduce what we consider to be acceptable in the restart field.

@jrha
Copy link
Member

jrha commented Oct 29, 2018

Consensus seems to be that this is too hard to solve and we should very strongly discourage use of subshells during PR reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants