evolix/evoadmin-web
22
0
Fork 1
Code Issues 31 Pull requests 4 Releases 1 Wiki Activity

New check command for web-add.sh #5

Merged
Ghost merged 5 commits from ticket-37722 into master 2018-12-13 23:29:07 +01:00
Conversation 13 Commits 5 Files changed 1 +44 -2
Ghost commented 2018-11-23 15:34:13 +01:00
First-time contributor
Copy link

I'd like to introduce a new fix command for scripts/web-add.sh,
which can be safely run when somebody overwrites symbolic links in
sites-enabled.

But there are many ways of doing this, it could be a command like
it is now, but it could also be a simple function that is called
anytime scripts/web-add.sh is run or a separate script that is
either run manually or through cron(8).

I'd like to introduce a new fix command for scripts/web-add.sh, which can be safely run when somebody overwrites symbolic links in sites-enabled. But there are many ways of doing this, it could be a command like it is now, but it could also be a simple function that is called anytime scripts/web-add.sh is run or a separate script that is either run manually or through cron(8).
benpro was assigned by Ghost 2018-11-23 15:34:13 +01:00
Ghost added the
Script
Bug
 labels 2018-11-23 15:34:13 +01:00
Ghost commented 2018-11-29 14:24:25 +01:00
Author
First-time contributor
Copy link

Any opinions or thoughts on this?

Any opinions or thoughts on this?
benpro commented 2018-11-29 15:07:40 +01:00
Contributor
Copy link

I commented the code with a "beware". The idea is good, but it should be used with precaution!

I commented the code with a "beware". The idea is good, but it should be used with precaution!
Ghost commented 2018-11-29 16:19:31 +01:00
Author
First-time contributor
Copy link

@benpro How do you view comments on the code?

@benpro How do you view comments on the code?
Ghost reviewed 2018-11-29 17:14:15 +01:00
Ghost reviewed 2018-11-29 17:16:47 +01:00
benpro reviewed 2018-11-29 17:18:05 +01:00
scripts/web-add.sh Outdated
@ -991,0 +997,4 @@
# Some people forget to use the --follow-symlinks flag with sed(1),
# thus not carrying changes over to /etc/sites-available.
op_fixvhosts() {
ln_vhosts_dir="$(echo "$VHOST_PATH" | sed 's/available/enabled')"
benpro commented 2018-11-23 15:46:37 +01:00
Contributor
Copy link

Useless echo? Missing last sed trail?

Should be:

sed 's/available/enabled/' <<< "$VHOST_PATH"
Useless echo? Missing last sed trail? Should be: ```bash sed 's/available/enabled/' <<< "$VHOST_PATH" ```
Ghost commented 2018-11-29 17:20:51 +01:00
Author
First-time contributor
Copy link

Mostly me not completely used to bash again, I'll fix it.

Mostly me not completely used to bash again, I'll fix it.
scripts/web-add.sh Outdated
@ -991,0 +1004,4 @@
do
vhost_name=$(basename "$ln_path")
mv "$ln_path" "$VHOST_PATH/$vhost_name"
benpro commented 2018-11-23 15:49:44 +01:00
Contributor
Copy link

Beware that operation can be dangerous. If people have modified the version in sites-available and sites-enabled, you should first merge the difference in the sites-available version, then do the symbolic link.

Beware that operation can be dangerous. If people have modified the version in sites-available **and** sites-enabled, you should first merge the difference in the sites-available version, then do the symbolic link.
Ghost commented 2018-11-29 17:21:46 +01:00
Author
First-time contributor
Copy link

I'm thinking it could probably take a --dry-run flag

I'm thinking it could probably take a --dry-run flag
Ghost commented 2018-11-30 16:33:43 +01:00
Author
First-time contributor
Copy link

So what would the behavior be when using it from the web interface?

So what would the behavior be when using it from the web interface?
benpro commented 2018-11-30 16:55:53 +01:00
Contributor
Copy link

Huh. No need for that feature in the web interface.
This is for helping sysadmins only.

Huh. No need for that feature in the web interface. This is for helping sysadmins only.
benpro commented 2018-11-30 16:56:33 +01:00
Contributor
Copy link

Yes you should add a --dry-run, or even should be the default.

Yes you should add a `--dry-run`, or even should be the default.
Ghost commented 2018-12-03 16:11:19 +01:00
Author
First-time contributor
Copy link

What's your preference: --force or --apply?

What's your preference: `--force` or `--apply`?
benpro commented 2018-12-03 16:15:41 +01:00
Contributor
Copy link

--force is nice. (If you force, you must understand the consequences ;))

`--force` is nice. (If you force, you must understand the consequences ;))
Ghost commented 2018-12-03 16:56:00 +01:00
Author
First-time contributor
Copy link

Yeah but web-add.sh fix --apply makes more semantic sense in my mind, web-add.sh fix --force seems to apply more to an operation that was aborted because of inconsistencies, but gives you the option to force break stuff? Like web-add.sh add-alias [alias] --force would make sense.

Yeah but `web-add.sh fix --apply` makes more semantic sense in my mind, `web-add.sh fix --force` seems to apply more to an operation that was aborted because of inconsistencies, but gives you the option to force break stuff? Like `web-add.sh add-alias [alias] --force` would make sense.
benpro commented 2018-12-03 17:10:45 +01:00
Contributor
Copy link

Well... Ok for --apply.

Well... Ok for `--apply`.
Ghost commented 2018-12-04 17:28:51 +01:00
Author
First-time contributor
Copy link

See the latest commit for changes. I switched fix-vhosts to check-vhosts and added the -f flag to apply fixes.

See the latest commit for changes. I switched fix-vhosts to check-vhosts and added the -f flag to apply fixes.
scripts/web-add.sh Outdated
@ -991,0 +1005,4 @@
vhost_name=$(basename "$ln_path")
mv "$ln_path" "$VHOST_PATH/$vhost_name"
ln -s "$VHOST_PATH/$vhost_name" "$ln_path"
benpro commented 2018-11-23 15:50:32 +01:00
Contributor
Copy link

You could also use a2ensite in place of doing a manual link.

You could also use `a2ensite` in place of doing a manual link.
Ghost commented 2018-11-29 17:22:04 +01:00
Author
First-time contributor
Copy link

Good idea

Good idea
Ghost commented 2018-12-03 16:12:07 +01:00
Author
First-time contributor
Copy link

Web wise I see the web-add.sh use the fix command before doing anything with an existing vhost, if it detects that theres a problem with the vhosts it should abort and tell the webpage to show "Contact your administrator, something is wrong with the vhosts".

Alternatively this functionality could also be exported to evocheck but it would imply adding a dependency to evocheck for evoadmin-web.

Web wise I see the `web-add.sh` use the fix command before doing anything with an existing vhost, if it detects that theres a problem with the vhosts it should abort and tell the webpage to show "Contact your administrator, something is wrong with the vhosts". Alternatively this functionality could also be exported to `evocheck` but it would imply adding a dependency to `evocheck` for `evoadmin-web`.
benpro commented 2018-12-03 16:14:47 +01:00
Contributor
Copy link

[…] if it detects that theres a problem with the vhosts it should abort and tell the webpage to show “Contact your administrator, something is wrong with the vhosts”.

I agree.

Alternatively this functionality could also be exported to evocheck […]

Evocheck is already checking that with IS_APACHESYMLINK.

> […] if it detects that theres a problem with the vhosts it should abort and tell the webpage to show “Contact your administrator, something is wrong with the vhosts”. I agree. > Alternatively this functionality could also be exported to `evocheck` […] Evocheck is already checking that with `IS_APACHESYMLINK`.
Ghost commented 2018-12-03 16:57:48 +01:00
Author
First-time contributor
Copy link

So should we be just using that? Evocheck could be modified to add a --fix flag that automatically fixes specific problems. I'll have to look at the existing code first though.

So should we be just using that? Evocheck could be modified to add a --fix flag that automatically fixes specific problems. I'll have to look at the existing code first though.
benpro commented 2018-12-03 17:05:24 +01:00
Contributor
Copy link

No Evocheck isn't meant to fix things.

No Evocheck isn't meant to fix things.
Ghost commented 2018-12-03 18:14:12 +01:00
Author
First-time contributor
Copy link

Fair enough, but should the code that checks if a fix is necessary use evocheck instead of my current implementation?

Fair enough, but should the code that checks if a fix is necessary use evocheck instead of my current implementation?
Ghost added the
Feature
 label 2018-12-04 17:29:35 +01:00
Ghost commented 2018-12-04 17:34:03 +01:00
Author
First-time contributor
Copy link

@benpro what do you think of the latest commit? (I'm not too sure how notifs work yet)

@benpro what do you think of the latest commit? (I'm not too sure how notifs work yet)
benpro reviewed 2018-12-05 09:46:12 +01:00
scripts/web-add.sh
benpro commented 2018-12-05 09:46:12 +01:00
Contributor
Copy link

"/ServerName .*/a \\\tServerAlias $alias missing the trailing slash?

`"/ServerName .*/a \\\tServerAlias $alias` missing the trailing slash?
benpro commented 2018-12-05 09:52:14 +01:00
Contributor
Copy link

LGTM

LGTM
Ghost commented 2018-12-05 15:04:33 +01:00
Author
First-time contributor
Copy link

Cool, @vlaborie what do you think? Anything sticking out?

If not I'll do one last round of testing.

Cool, @vlaborie what do you think? Anything sticking out? If not I'll do one last round of testing.
Ghost changed title from WIP: New fix command for web-add.sh to New check command for web-add.sh 2018-12-05 15:04:46 +01:00
Ghost commented 2018-12-13 19:24:15 +01:00
Author
First-time contributor
Copy link

@romain any comments or is it pas pire?

@romain any comments or is it pas pire?
Ghost commented 2018-12-13 22:57:19 +01:00
Author
First-time contributor
Copy link

Sounds good for me, you can merge your code.

Sounds good for me, you can merge your code.
Ghost closed this pull request 2018-12-13 23:29:07 +01:00
Ghost deleted branch ticket-37722 2018-12-13 23:29:17 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Bug

Fixing or reporting bugs

  
Doc

Things that needs to be documented (use and/or dev)

  
Feature

Adding new features

  
Forge

Issues imported from https://forge.evolix.org/projects/evoadmin-web/issues

Archived

  
Mode
Cluster

  
Mode
MultiPHP

  
Script

Code that can be run both on the command line or by evoadmin

  
Server

Server configuration

  
Web

Code that is run on or by the http server.

  
wontfix
No labels
Bug
Doc
Feature
Forge
Mode
Cluster
Mode
MultiPHP
Script
Server
Web
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
benpro
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: evolix/evoadmin-web#5
No description provided.
Delete branch "ticket-37722"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?