greenled / portainer-stack-utils

CLI client for Portainer
https://hub.docker.com/r/greenled/portainer-stack-utils/
GNU General Public License v3.0
74 stars 16 forks source link

improve and simplifying quotes excape #33

Closed vinsgithub closed 5 years ago

vinsgithub commented 5 years ago

fixing quote excape. As a test case (in a docker compose) you can try this one

version: '3'
services:
  mydb:
    image: mysql:8.0.13
    environment:
      - MYSQL_USER=user
      - MYSQL_PASSWORD=secret
      - MYSQL_ROOT_PASSWORD=password
    entrypoint:
      sh -c "
        echo 'CREATE DATABASE IF NOT EXISTS mydb;     GRANT ALL PRIVILEGES ON mydb.*     TO §uer§@§%§; ' > /docker-entrypoint-initdb.d/init.sql;
        sed -i 's/§/\\d39/g' /docker-entrypoint-initdb.d/init.sql;
        /usr/local/bin/docker-entrypoint.sh --character-set-server=utf8 --collation-server=utf8_unicode_ci --default-authentication-plugin=mysql_native_password
      "
    volumes:
      - mydb:/var/lib/mysql
    ports:
      - "3307:3306"
greenled commented 5 years ago

Hi @vinsgithub! Thank you for your interest in giving back to the project. I'm not able to test your PR right now, will but at a first sight it looks like you are delegating the quote escaping to jq. Is that right? Please, could you elaborate a bit on why are you proposing this change? Maybe the current implementation fails with the test case you provide, or yours is just easier to read (which actually is :+1:).

greenled commented 5 years ago

@tortuetorche wanna take a look at this one? I think adding this doesn't get in conflict with your work on several-changes branch.

vinsgithub commented 5 years ago

Hi @vinsgithub! Thank you for your interest in giving back to the project. I'm not able to test your PR right now, will but at a first sight it looks like you are delegating the quote escaping to jq. Is that right? Please, could you elaborate a bit on why are you proposing this change? Maybe the current implementation fails with the test case you provide, or yours is just easier to read (which actually is +1).

Exactly. I used the current implementation on my use case ("sh" instructions with many quotes and double quotes combinations) and it didn't work. Then I realized there was a very "simple" quote/s management in psu script and looked at how jq could help with that. Now it works great.

greenled commented 5 years ago

Hi @vinsgithub. Sorry for the long wait, I finally made some time to review your patch. A far as I can see lines like sed -i 's/§/\\d39/g' are sent as sed -i 's/§/\d39/g' (note the missing \) in the current implementation. Your patch fixes those backslash escaping cases (and probably some other unspotted escaping issues) through jq, and makes the code more readable and maintainable. Thanks a lot!

greenled commented 5 years ago

Just in case someone else tries to test the example stack you provide, I have to note it's missing some parts (volume, db user) to be successfully deployed, but it's enough to show the escaping issue in psu and it's fix.

vinsgithub commented 5 years ago

@greenled thanks. is it possible for you to build and push also docker image with this new version of psu? I find very useful running through container. Thank you,

greenled commented 5 years ago

@vinsgithub sure. I just added the 0.1.2 tag to https://hub.docker.com/r/greenled/portainer-stack-utils.