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

Add ynh_systemd_action helper #633

Open
wants to merge 10 commits into
base: stretch-unstable
from

Conversation

Projects
None yet
4 participants
@maniackcrudelis
Copy link
Contributor

maniackcrudelis commented Jan 28, 2019

The problem

I use this helper in all my own apps without any problem.
That's a really useful helper, especially for all our helpers with a systemctl restart or reload. By using this helper, we'll have directly the log when a service fail to start.

Solution

Add ynh_systemd_action as an official helper.

PR Status

Ready to be reviewed.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

maniackcrudelis added some commits Jan 28, 2019

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Jan 28, 2019

Show resolved Hide resolved data/helpers.d/system
Show resolved Hide resolved data/helpers.d/system Outdated

@alexAubin alexAubin added this to the 3.5.x milestone Feb 5, 2019

@alexAubin
Copy link
Member

alexAubin left a comment

Quick review, didn't test, agreeing on the principle

That indeed looks quite useful (especially the --line_match)

maniackcrudelis and others added some commits Feb 6, 2019

Show resolved Hide resolved data/helpers.d/backend Outdated
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 15, 2019

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Feb 16, 2019

Well, I discovered some issues with this helper.

So I'll push some fix when I's tested

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Feb 16, 2019

I've pushed some small changes.

For me it's OK now.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 16, 2019

What were the issues @Josue-T ?
Instead of 'fixed_string', did you try to simply escape characters !? Otherwise, we'll need that in almost every helpers !
And why adding 'systemd_log' ? That was already covered by $log_path at 'systemd'.

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Feb 16, 2019

For the fixed_string, I think It make really more easy to just add this option like it is. Probably escaping the caractere should also work.

And for the option "systemd" it didn't work because if the service fail to start, we get an error like can't find the file "systemd".

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 16, 2019

I'd rather prefer to keep the usage of escaping characters...

I had seen also that error about systemd, but quickly and had never investigated it.
So your idea would be either to use --log_path or --systemd_log ? If so, it's clearly not going to work since log_path has a default value...

@Josue-T Josue-T force-pushed the add_ynh_systemd_action branch from 4001745 to af82be4 Feb 16, 2019

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 16, 2019

Huh, so what you revert everything ?
Do you think it would be possible to discuss before commiting when you find something ? Instead of just pushing commits without explaining before.

Show resolved Hide resolved data/helpers.d/system Outdated
Show resolved Hide resolved data/helpers.d/system Outdated

maniackcrudelis and others added some commits Feb 17, 2019

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