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 list_ for presense detection #10

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix list_ for presense detection #10

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e589861
Fix list_ for presense detection
JesperBeltman Oct 17, 2024
d1a8090
docs: Add changelog issue 9
JesperBeltman Oct 24, 2024
7b5ceba
Revert and adjust list function
JesperBeltman Nov 12, 2024
8ad5fa2
fix: No newline at end of file
JesperBeltman Nov 12, 2024
ea4a069
Update changelog/9.fixed.md
JesperBeltman Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/9.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed presence detection with using the mysql_cache module and removal of create table function
JesperBeltman marked this conversation as resolved.
Show resolved Hide resolved
22 changes: 14 additions & 8 deletions src/saltext/mysql/cache/mysql_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class InterfaceError(Exception):
# Module properties

__virtualname__ = "mysql"
__func_alias__ = {"ls": "list"}
__func_alias__ = {"list_": "list"}


def __virtual__():
Expand Down Expand Up @@ -309,17 +309,23 @@ def flush(bank, key=None):
cur.close()


def ls(bank):
def list_(bank):
"""
Return an iterable object containing all entries stored in the specified
bank.
Return an iterable object containing all entries stored in the specified bank.
"""
bank_like = bank + '%'
Copy link
Member

Choose a reason for hiding this comment

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

note: This should work with and without trailing /

Suggested change
bank_like = bank + '%'
bank = bank.rstrip("/")
bank_like = bank + "/%"

Copy link
Author

Choose a reason for hiding this comment

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

I did not know, that in the code they are using a trailing /
But its easy to handle like this indeed.

_init_client()
query = "SELECT etcd_key FROM {} WHERE bank=%s".format(__context__["mysql_table_name"])
cur, _ = run_query(__context__.get("mysql_client"), query, args=(bank,))
banks_query = "SELECT bank FROM {} WHERE bank LIKE %s".format(
__context__["mysql_table_name"]
)
cur, _ = run_query(__context__.get("mysql_client"), banks_query, args=(bank_like,))
out = [row[0] for row in cur.fetchall()]
cur.close()
return out
data = []
Copy link
Member

@lkubb lkubb Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

issue(blocking): This does not deduplicate, is missing files and does not list subdirectories correctly. Take the following structure (key == etc_key for brevity):

- bank: root/sub/dir
  key: foo
- bank: root/sub/dir
  key: bar
- bank: root/another_sub/dir
  key: baz
- bank: root
  key: file

Listing root should yield:

- sub
- another_sub
- file

But this returns:

- dir
- dir
- dir

Please see the last paragraph of my previous review comment for suggestions.

Edit: Noticed another issue caused by returning the last path segment. You need to strip the bank that is being listed from the entry, then return the first path segment only. If you fix this, the function would still incorrectly return:

- sub
- sub
- another_sub

Thus, I'd use a set for data and only cast it to a list when returning from the function, which would yield:

- sub
- another_sub

The last thing that's missing is etcd_key entries where the bank entry matches the bank parameter exactly.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the review. I'm thinking of using the last part returned by mysql of the bank this should contain sub, another_sub, file. Using sets sounds like a great idea to fix the duplicate entries. Does it happen the etcd_key that matches the full bank parameter, perhaps it could be fixed by one SQL query without LIKE. I really don't like the idea of having another SQL query.

I'm grateful for all that you have done, I will do some thinking of my own.

Copy link
Member

@lkubb lkubb Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It should work with a single query similar to this mockup (definitely needs adjustments, e.g. client initialization, possibly the for loop):

def list_(bank):
    bank = bank.strip("/")
    query = "SELECT bank, etcd_key FROM {} WHERE bank LIKE %s".format(
        __context__["mysql_table_name"]
    )
    cur, _ = run_query(__context__.get("mysql_client"), keys_query, args=(bank,))
    res = set()
    for res_bank, res_key in cur.fetchall():
        if res_bank == bank:
            res.add(res_key)
        else:
            res.add(res_bank[len(bank) + 1:].split("/")[0])
    return list(res)

for entry in out:
filtered_entry = entry.split('/')[-1]
data.append(filtered_entry)
return data


def contains(bank, key):
Expand Down Expand Up @@ -354,4 +360,4 @@ def updated(bank, key):
cur, _ = run_query(__context__.get("mysql_client"), query=query, args=data)
r = cur.fetchone()
cur.close()
return int(r[0]) if r else r
return int(r[0]) if r else r
Loading