Skip to content

Fix node filter to use EXISTS for exact hostname matches #353

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

Merged
merged 1 commit into from
Apr 8, 2025

Conversation

brinkcoder
Copy link

This PR fixes the node filter to use exact hostname matches instead of substring matches with LIKE, preventing issues like node=cpu001 matching fatcpu001.

Changes:

  • internal/repository/jobQuery.go: Updated BuildWhereClause to use EXISTS for node.Eq and node.Contains.
  • internal/repository/jobFind.go: Updated FindConcurrentJobs to use EXISTS.

@spacehamster87
Copy link
Contributor

Hi there and thanks for this PR!

While the PR solves the lingering issue, the contains query fix would conflict with the current definition of the filter keyword.

This is because contains is supposed to be a LIKE operator on the DB side, as in "DB field contains this partial string".
That being said, using contains within the frontend is suboptimal in the first place, so thanks for pointing this out!

With some tweaks on the frontend side, it is possible to use both eq (exact match, should be default) and contains operators in the frontend filters, and handling them in the backend accordingly. Maybe even exchange contains for eq in all frontend use-cases.

@moebiusband73 : I would suggest changing the target branch to dev, so I can adapt the frontend to the requested changes there instead of master.

@brinkcoder
Copy link
Author

You are absolutely right. I changed the contains to a LIKE operator.

This change repairs the filtering for Resources but breaks the Concurrent Job List again (first commit had Concurrent List right but filtering was broken).

@moebiusband73
Copy link
Member

Hi, we need to review and adopt other things for this. Can you change the target branch to the dev branch? Otherwise I can cancel the pull request and we can implement this based on what you provided.
What do you think?
Best, Jan

@brinkcoder brinkcoder changed the base branch from master to dev April 4, 2025 10:28
@brinkcoder
Copy link
Author

Hi, I thought you had to change the branch. Just noticed that I can do it myself.

@spacehamster87
Copy link
Contributor

Thanks - I will merge now and fix the frontend issue

@spacehamster87 spacehamster87 merged commit b3a1037 into ClusterCockpit:dev Apr 8, 2025
1 check passed
@spacehamster87 spacehamster87 self-assigned this Apr 8, 2025
@brinkcoder brinkcoder deleted the fix-node-filter branch April 9, 2025 14:47
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.

3 participants