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

Fixed the overlay command in Bastillefile #231

Merged
merged 2 commits into from
Aug 20, 2020
Merged

Fixed the overlay command in Bastillefile #231

merged 2 commits into from
Aug 20, 2020

Conversation

tobiastom
Copy link
Contributor

The documentation did mention that OVERLAY should be used, but only cp was implemented.
Also the destination part where to copy the files to has been missing.

The previous hooks seem to implement it in a similar way: https://github.com/BastilleBSD/bastille/blob/master/usr/local/share/bastille/template.sh#L175

@chriswells0
Copy link
Collaborator

I'm the one responsible for creating these bugs.

The intention was to create a CP alias for OVERLAY, but I forgot to use cp|overlay). I think it would be nice to have that as an option instead of just changing it to overlay. COPY is easier to read, but I used CP for consistency with the existing command. I don't know if @cedwards would want to weigh in on those points (both having an alias in general and whether to use CP or COPY).

As for the missing slash, I was providing a destination path in my testing:

CP www/ usr/local/www/nginx-dist/

I see why supporting no 2nd parameter for backward compatibility makes sense, but it would be great to also support a 2nd parameter to have the ability to specify a destination. It's kind of a pain to implement taking different possibilities into consideration: 1 param or 2, either/both of them quoted with spaces, either/both having a leading slash. This function seems to work, but please double check me:

make_paths_absolute() {
    _absolute_paths=''

    # First parameter: "from" path.
    if [ "${1%${1#?}}" = '/' ]; then # Absolute path, so just ensure it's quoted. -- cwells
        _absolute_paths="\"${1}\""
    else # Convert relative "from" path into absolute path inside the template directory. -- cwells
        _absolute_paths="\"${bastille_template}/${1}\""
    fi

    # Second parameter: "to" path.
    if [ -z "$2" ]; then # Not provided, so assume root. -- cwells
        _absolute_paths="${_absolute_paths} /"
    elif [ "${2%${2#?}}" = '/' ]; then # Absolute path, so just ensure it's quoted. -- cwells
        _absolute_paths="${_absolute_paths} \"${2}\""
    else # Convert relative "to" path into absolute path inside the jail. -- cwells
        _absolute_paths="${_absolute_paths} \"/${2}\""
    fi

    echo "$_absolute_paths"
}

Then you have to call it using eval to parse the single string containing both paths into 2 strings:

cp|overlay)
	_cmd='cp'
	_args=$(eval "make_paths_absolute $_args")
	;;

Would you mind using that or something similar to get both working?

@cedwards cedwards self-assigned this Aug 5, 2020
@tobiastom
Copy link
Contributor Author

Would it help to merge this if I implement it like you described above?

@cedwards
Copy link
Contributor

It doesn't need to be implemented like above but we should support both cp and overlay in the template cases. cp provides both source and dest args, while overlay provides the source path and assumes /(root) of container.

@tobiastom
Copy link
Contributor Author

@chriswells0 Would you mind looking at this again. I fixed it very basic right now for two reasons:

  1. I like the separation between overlay and copy. Not sure if everybody will share this opinion
  2. even if I like your implementation of make_paths_absolute, I'm not sure if it's trying to be to smart.

With this one copy needs two arguments, and overlay one.

Also, I added an alias copy for cp.

@cedwards
Copy link
Contributor

I think this looks pretty good. "Basic" is (usually) the goal.

@cedwards cedwards merged commit 8b196ff into BastilleBSD:master Aug 20, 2020
@tobiastom tobiastom deleted the bugfix/templates branch August 21, 2020 07:16
@chriswells0
Copy link
Collaborator

Sorry for the delay: going through some busy times.

Thanks for keeping the 2-parameter version of CP/COPY. I like the simplicity of knowing I can use any Bastille subcommand in a Bastillefile.

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.

None yet

3 participants