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

Correct validation #9

Merged
merged 2 commits into from
Aug 6, 2018
Merged

Correct validation #9

merged 2 commits into from
Aug 6, 2018

Conversation

kirtangajjar
Copy link
Contributor

Signed-off-by: Kirtan Gajjar [email protected]

Signed-off-by: Kirtan Gajjar <[email protected]>
@kirtangajjar kirtangajjar requested a review from mbtamuli July 27, 2018 15:02
@kirtangajjar kirtangajjar requested a review from mrrobot47 July 27, 2018 15:03
Copy link
Member

@mrrobot47 mrrobot47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kirtangajjar There is no need to add validation for # inside a cron-job command checks.

@mrrobot47
Copy link
Member

mrrobot47 commented Jul 30, 2018

@kirtangajjar There is no need to check for # inside a cron-job command. If there is a # then it means that anything after it will not be executed and is a comment only. That is the default behavior for crons or commands in linux.
Also, most importantly there is no need to throw error if someone adds a comment using # in the cron-command!

@mrrobot47 mrrobot47 closed this Jul 30, 2018
@kirtangajjar
Copy link
Contributor Author

@mrrobot47 What if the # is inside a string?
i.e. echo 'abc#234'?

I talked with @mbtamuli about it and that's why we need this.

@kirtangajjar kirtangajjar reopened this Jul 30, 2018
@mbtamuli
Copy link
Contributor

You can track the fix for this in #10.

Until it is fixed, we can only show a error message.

Signed-off-by: Riddhesh Sanghvi <[email protected]>
@mbtamuli mbtamuli merged commit 5133ce1 into develop Aug 6, 2018
@mbtamuli mbtamuli deleted the validate_command branch August 9, 2018 14:29
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.

3 participants