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.

Sign up for GitHub

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

Jump to bottom

fix(libtofs): use select to deal with empty strings in path #124

Merged
merged 2 commits into from
May 29, 2019
Merged

fix(libtofs): use select to deal with empty strings in path #124

merged 2 commits into from
May 29, 2019

Conversation

myii
Copy link
Member

@myii myii commented May 23, 2019 •
edited
Loading

  • To prevent path breakage when any of the following settings are used
    in the pillar/config:
  tofs:
    path_prefix: ''
    dirs:
      files:     ''
      default:   ''

This PR is based on some discussions in our Slack/IRC/Matrix room, where @sticky-note has been looking at how TOFS can be introduced to the nginx-formula (leading to saltstack-formulas/nginx-formula#238). The above pillar was set and led to some breakages, which I will cover in subsequent comments in this PR.

Edit by @n-rodriguez: the diff is in the double slash // before and after. Great job 👍

@myii
fix(libtofs): use select to deal with empty strings in path
afe0751 
* To prevent path breakage when any of the following settings are used
  in the pillar/config:

```
  tofs:
    path_prefix: ''
    dirs:
      files:     ''
      default:   ''
```
@myii myii requested review from baby-gnu and aboe76 May 23, 2019 13:10
@myii
Copy link
Member Author

myii commented May 23, 2019 •
edited
Loading

Refactor: same results

No changes introduced when dealing with expected pillar/config configurations.

Commented out

  tofs:
    # path_prefix: ''
    # dirs:
    #   files:     ''
    #   default:   ''

Existing:

    - source:
      - salt://nginx/files/ABC/server.conf
      - salt://nginx/files/Debian/server.conf
      - salt://nginx/files/default/server.conf

New:

    - source:
      - salt://nginx/files/ABC/server.conf
      - salt://nginx/files/Debian/server.conf
      - salt://nginx/files/default/server.conf

Using alt values

  tofs:
    path_prefix: 'alt_path_prefix'
    dirs:
      files:     'alt_files'
      default:   'alt_default'

Existing:

    - source:
      - salt://alt_path_prefix/alt_files/ABC/server.conf
      - salt://alt_path_prefix/alt_files/Debian/server.conf
      - salt://alt_path_prefix/alt_files/alt_default/server.conf

New:

    - source:
      - salt://alt_path_prefix/alt_files/ABC/server.conf
      - salt://alt_path_prefix/alt_files/Debian/server.conf
      - salt://alt_path_prefix/alt_files/alt_default/server.conf

@myii
Copy link
Member Author

myii commented May 23, 2019 •
edited
Loading

Fix: blank default

Broken path with existing configuration.

Pillar/config

  tofs:
    # path_prefix: ''
    dirs:
    #   files:     ''
      default:   ''

Existing:

    - source:
      - salt://nginx/files/ABC/server.conf
      - salt://nginx/files/Debian/server.conf
      - salt://nginx/files//server.conf

New:

    - source:
      - salt://nginx/files/ABC/server.conf
      - salt://nginx/files/Debian/server.conf
      - salt://nginx/files/server.conf

Diff:

-      - salt://nginx/files//server.conf
+      - salt://nginx/files/server.conf

@myii
Copy link
Member Author

myii commented May 23, 2019 •
edited
Loading

Fix: blank files

Broken path with existing configuration.

Pillar/config

  tofs:
    # path_prefix: ''
    dirs:
      files:     ''
    #   default:   ''

Existing:

    - source:
      - salt://nginx//ABC/server.conf
      - salt://nginx//Debian/server.conf
      - salt://nginx//default/server.conf

New:

    - source:
      - salt://nginx/ABC/server.conf
      - salt://nginx/Debian/server.conf
      - salt://nginx/default/server.conf

Diff:

-      - salt://nginx//ABC/server.conf
-      - salt://nginx//Debian/server.conf
-      - salt://nginx//default/server.conf
+      - salt://nginx/ABC/server.conf
+      - salt://nginx/Debian/server.conf
+      - salt://nginx/default/server.conf

@myii
Copy link
Member Author

myii commented May 23, 2019 •
edited
Loading

Fix: blank path_prefix

Broken path with existing configuration.

Pillar/config

  tofs:
    path_prefix: ''
    # dirs:
    #   files:     ''
    #   default:   ''

Existing:

    - source:
      - salt:///files/ABC/server.conf
      - salt:///files/Debian/server.conf
      - salt:///files/default/server.conf

New:

    - source:
      - salt://files/ABC/server.conf
      - salt://files/Debian/server.conf
      - salt://files/default/server.conf

Diff:

-      - salt:///files/ABC/server.conf
-      - salt:///files/Debian/server.conf
-      - salt:///files/default/server.conf
+      - salt://files/ABC/server.conf
+      - salt://files/Debian/server.conf
+      - salt://files/default/server.conf

@myii
Copy link
Member Author

myii commented May 23, 2019 •
edited
Loading

Fix: everything blank

Broken path with existing configuration.

Pillar/config

  tofs:
    path_prefix: ''
    dirs:
      files:     ''
      default:   ''

Existing:

    - source:
      - salt:////ABC/server.conf
      - salt:////Debian/server.conf
      - salt://///server.conf

New:

    - source:
      - salt://ABC/server.conf
      - salt://Debian/server.conf
      - salt://server.conf

Diff:

-      - salt:////ABC/server.conf
-      - salt:////Debian/server.conf
-      - salt://///server.conf
+      - salt://ABC/server.conf
+      - salt://Debian/server.conf
+      - salt://server.conf

@n-rodriguez
Copy link
Member

And now... enter in the formulas update hell 😄

@n-rodriguez
Copy link
Member

n-rodriguez commented May 23, 2019 •
edited
Loading

Maybe it could be solved by a script launched by someone?

  1. get the list of tofs repos
  2. clone repos
  3. copy file
  4. git add && git commit && git push
  5. create PR manually :)

The script could reside in template-formula repo with a yaml file to store repos list (tofs, semantic-release)

@myii
Copy link
Member Author

myii commented May 23, 2019

Thanks for the review @n-rodriguez -- and the comment at the very top!

@myii
Copy link
Member Author

myii commented May 23, 2019

@n-rodriguez I hit the send button prematurely there (hit Ctrl-Enter by mistake). Anyway, a brief comment about the "update hell": thankfully, this PR isn't critical, so we don't have to worry about it yet! But on a serious note, this is something that has been considered and there are various ways it can be done. I'm in favour of automating it as much as possible (eventually) and there are tools out there that can pretty much do the steps for you (the ones you have laid out above). We'd need to be able to do it from a higher permissions level so we'd need to propose it upstream. One example of something that could be used in the interim: git bulk.

@myii
Copy link
Member Author

myii commented May 23, 2019

@n-rodriguez I decided to edit my examples to provide a clearer diff as well for each one -- thanks for that comment.

@n-rodriguez
Copy link
Member

@myii awesome!

@myii
Copy link
Member Author

myii commented May 24, 2019

@baby-gnu Maybe selecting you for a review doesn't ping you properly, so I'm trying again with this comment! You are ideally placed to judge if this is a useful PR or not. If it is, we can use some of the strategies laid out above to propagate the changes to all of the other formulas using libtofs.jinja.

@myii
Copy link
Member Author

myii commented May 25, 2019

Actually, if this is agreed upon, should the document be updated to include some of these examples?

This was referenced May 26, 2019
Open
Merged
baby-gnu
baby-gnu requested changes May 27, 2019
View reviewed changes
Copy link
Contributor

@baby-gnu baby-gnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the src_file.lstrip('/'), I'm fine with this pull request.

template/libtofs.jinja Show resolved Hide resolved
baby-gnu
baby-gnu approved these changes May 29, 2019
View reviewed changes
Copy link
Contributor

@baby-gnu baby-gnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to strip / from all path components.

@myii
Copy link
Member Author

myii commented May 29, 2019

Thanks for the review @baby-gnu.

@myii myii merged commit 5945ed9 into saltstack-formulas:master May 29, 2019
@myii myii deleted the bug/libtofs-select-join branch May 29, 2019 13:36
@myii
Copy link
Member Author

myii commented May 29, 2019

I've gone ahead and merged this, since it's needed in some of the TOFS conversion PRs that are in progress right now. Thanks all.

@saltstack-formulas-travis
Copy link

🎉 This PR is included in version 2.1.18 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

@baby-gnu baby-gnu baby-gnu approved these changes

@n-rodriguez n-rodriguez n-rodriguez approved these changes

@aboe76 aboe76 Awaiting requested review from aboe76

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@myii @n-rodriguez @saltstack-formulas-travis @baby-gnu