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

Show the users the starting point of the execution of the PRE/POS_RECOVERY_SCRIPTs #2789

Closed
wants to merge 1 commit into from

Conversation

ivarmu
Copy link
Contributor

@ivarmu ivarmu commented Apr 13, 2022

Relax-and-Recover (ReaR) Pull Request Template

Please fill in the following items before submitting a new pull request:

Pull Request Details:
  • Type: Enhancement

  • Impact: Low

  • Reference to related issue (URL): No issue created

  • How was this pull request tested?: Local VM

  • Brief description of the changes in this pull request: Add a LogPrint to show the user the scripts are being executed

@pcahyna
Copy link
Member

pcahyna commented Apr 13, 2022

Hi, thanks for the change, I believe it would be better to do this after PR #2735 is merged, otherwise the changes would conflict. Also, is it intentional that it is done only for PRE/POST_RECOVERY_SCRIPT and not for PRE/POST_BACKUP_SCRIPT ?
I suppose you are using the scripts yourself, could you please have a look at PR #2735 and check if it is useful for you, including the suggestion I made at #2735 (comment) ? If you don't find any issue with that PR, I would like to merge it soon and then update and merge this one.

@ivarmu
Copy link
Contributor Author

ivarmu commented Apr 13, 2022

I'm ok with merging that PR after #2735. The mine one is introducing only information to the output in run time... so it's OK. What I can do is to add the same Information for PRE/POST_BACKUP_SCRIPT.

What do you think?

@pcahyna
Copy link
Member

pcahyna commented Apr 14, 2022

Yes, I would do the same change for PRE/POST_BACKUP_SCRIPT.

What do you think about the changes in PR #2735 and the suggestion in the last comment there? (I think we should gradually deprecate PRE/POST_*_SCRIPT and replace by the proposed PRE/POST_*_COMMANDS.)

@ivarmu
Copy link
Contributor Author

ivarmu commented Apr 14, 2022

I don't agree to add that new variables, but may maintain the original ones:

  • PRE/POST_*_SCRIPT: I understand it to a unique (and external to ReaR) script to be executed as is (the original intention I think)
  • PRE/POST_*_COMMANDS: It is supposed to be able to run one or more commands, not necessarily a script. These commands are external to ReaR, too, but adds more complexity to ReaR when is not needed, as a single script can contain all the commands to be executed.

So, in my honest opinion, ReaR is not intended to re-implement a shell to parse all the shell grammar, letting to run an external script should be enough.

@jsmeix jsmeix added the enhancement Adaptions and new features label Apr 20, 2022
jsmeix added a commit that referenced this pull request Jun 1, 2022
Include the changes from #2789
i.e. use LogPrint to show the user the executed commands
jsmeix added a commit that referenced this pull request Jun 1, 2022
Include the changes from #2789
i.e. use LogPrint to show the user the executed commands
@jsmeix
Copy link
Member

jsmeix commented Jun 1, 2022

I included the changes in this pull request
in my #2811

@jsmeix jsmeix added this to the ReaR v2.7 milestone Jun 2, 2022
jsmeix added a commit that referenced this pull request Jun 2, 2022
Add PRE_RECOVERY_COMMANDS and POST_RECOVERY_COMMANDS
as alternative to PRE_RECOVERY_SCRIPT and POST_RECOVERY_SCRIPT
see the description in default.conf how to use them and how they work.
See #2811 and see also
#2735 therein in particular
#2735 (comment)
Additionally use LogPrint to show the user the executed commands,
see #2789
@jsmeix
Copy link
Member

jsmeix commented Jun 2, 2022

With #2811 merged
this pull request got obsoleted by it.

@jsmeix jsmeix closed this Jun 2, 2022
@jsmeix
Copy link
Member

jsmeix commented Jun 2, 2022

@ivarmu
thank you for your enhancement pull request!

PRE_RECOVERY_SCRIPT and POST_RECOVERY_SCRIPT
are still there and did not change how they work
so you could use any combination of
PRE_RECOVERY_COMMANDS and POST_RECOVERY_COMMANDS
and PRE_RECOVERY_SCRIPT and POST_RECOVERY_SCRIPT
according to what works best for your needs.

I think in particular for longer complicated things
it is better to have them all in separated scripts
and call them via PRE_RECOVERY_SCRIPT and
POST_RECOVERY_SCRIPT.

But to "just run a few simple commands"
PRE_RECOVERY_COMMANDS and POST_RECOVERY_COMMANDS
are likely easier to use for the user because
he can specify all in his etc/rear/local.conf
and does not need to care about external scripts.

pcahyna pushed a commit to pcahyna/rear that referenced this pull request Feb 17, 2023
Add PRE_RECOVERY_COMMANDS and POST_RECOVERY_COMMANDS
as alternative to PRE_RECOVERY_SCRIPT and POST_RECOVERY_SCRIPT
see the description in default.conf how to use them and how they work.
See rear#2811 and see also
rear#2735 therein in particular
rear#2735 (comment)
Additionally use LogPrint to show the user the executed commands,
see rear#2789
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants