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

feat(server): archivedWf add namePrefix search. Fixes #6743 #6801

Merged

Conversation

tczhao
Copy link
Member

@tczhao tczhao commented Sep 25, 2021

Fixes #6743

image

Tips:

  • Maybe add you organization to USERS.md.
  • Your PR needs to pass the required checks before it can be approved. If the check is not required (e.g. E2E tests) it does not need to pass
  • Sign-off your commits to pass the DCO check: git commit --signoff.
  • Run make pre-commit -B to fix codegen or lint problems.
  • Say how how you tested your changes. If you changed the UI, attach screenshots.
  • If changes were requested, and you've made them, then dismis the review to get it looked at again.
  • You can ask for help!

@codecov
Copy link

codecov bot commented Sep 25, 2021

Codecov Report

Merging #6801 (5ced741) into master (689ad68) will increase coverage by 0.01%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6801      +/-   ##
==========================================
+ Coverage   48.55%   48.56%   +0.01%     
==========================================
  Files         265      265              
  Lines       19220    19228       +8     
==========================================
+ Hits         9332     9338       +6     
+ Misses       8839     8837       -2     
- Partials     1049     1053       +4     
Impacted Files Coverage Δ
persist/sqldb/migrate.go 0.00% <0.00%> (ø)
persist/sqldb/null_workflow_archive.go 0.00% <0.00%> (ø)
persist/sqldb/workflow_archive.go 0.00% <0.00%> (ø)
server/workflowarchive/archived_workflow_server.go 59.43% <100.00%> (+0.38%) ⬆️
...orkflow/controller/estimation/estimator_factory.go 70.00% <100.00%> (ø)
cmd/argoexec/commands/emissary.go 50.35% <0.00%> (-1.44%) ⬇️
workflow/controller/operator.go 71.11% <0.00%> (-0.10%) ⬇️
server/workflow/workflow_server.go 46.80% <0.00%> (+2.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 689ad68...5ced741. Read the comment docs.

@tczhao tczhao changed the title feat(server): archive search using LIKE operator on name feat(server): archivedWf name search using LIKE operator Sep 25, 2021
@tczhao tczhao marked this pull request as draft September 25, 2021 15:44
@tczhao tczhao marked this pull request as ready for review September 26, 2021 01:05
@tczhao tczhao changed the title feat(server): archivedWf name search using LIKE operator feat(server): archivedWf name search using LIKE operator. Fixes #6743 Sep 26, 2021
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

The use case here appears to be that the leading part of the name should match an expression.

To achieve this, you need name like 'prefix-%' condition.

I think this should be a separate query parameter name prefix and this is only used with like ${prefix}%. You'll need a new func named hasPrefix similar to nameEquals.

Do we have an index on name? Can you please check?

@tczhao
Copy link
Member Author

tczhao commented Sep 30, 2021

we don't have index on name
current indexes

    "argo_archived_workflows_pkey" PRIMARY KEY, btree (clustername, uid)
    "argo_archived_workflows_i1" btree (clustername, instanceid, namespace)
    "argo_archived_workflows_i2" btree (clustername, instanceid, finishedat)

would you recommend to
add (clustername, instanceid, name) index,
default the search bar behaviour to find through prefix matching,
and remove the exact match

@alexec
Copy link
Contributor

alexec commented Sep 30, 2021

would you recommend to add (clustername, instanceid, name) index,

yes. what about the labels table?

@tczhao tczhao force-pushed the feature/ui-archive-search-name-expression branch 3 times, most recently from 97109ba to b4f3334 Compare October 4, 2021 02:57
@tczhao tczhao requested a review from alexec October 4, 2021 03:18
@tczhao
Copy link
Member Author

tczhao commented Oct 6, 2021

Hi @alexec , any further comments?

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

I've reviewed the code and this is a breaking change, but we can modify it to remove the breaking change.

We should continue to support name=, via list.Option.fieldSelector=metadata.name and add namePrefix as another option to.

message ListArchivedWorkflowsRequest {
    k8s.io.apimachinery.pkg.apis.meta.v1.ListOptions listOptions = 1;
    string namePrefix = 2;
}

That means where we had name we now want name and namePrefix. If both are supplied, which would be odd, both must pass.

Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
@tczhao tczhao force-pushed the feature/ui-archive-search-name-expression branch from b4f3334 to c6776d2 Compare October 7, 2021 04:24
@tczhao tczhao requested a review from jessesuen as a code owner October 7, 2021 04:24
@tczhao
Copy link
Member Author

tczhao commented Oct 7, 2021

updated
image

Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
@tczhao tczhao changed the title feat(server): archivedWf name search using LIKE operator. Fixes #6743 feat(server): archivedWf add namePrefix search. Fixes #6743 Oct 7, 2021
@alexec alexec merged commit b548097 into argoproj:master Oct 7, 2021
@alexec
Copy link
Contributor

alexec commented Oct 7, 2021

Awesome. Looks a lot neater like this. Great work!

@sarabala1979 sarabala1979 mentioned this pull request Oct 15, 2021
19 tasks
@sarabala1979 sarabala1979 mentioned this pull request Oct 19, 2021
19 tasks
kriti-sc pushed a commit to kriti-sc/argo-workflows that referenced this pull request Oct 24, 2021
…rgoproj#6801)

Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
Signed-off-by: kriti-sc <kathuriakriti1@gmail.com>
@sarabala1979 sarabala1979 mentioned this pull request Nov 9, 2021
25 tasks
@sarabala1979 sarabala1979 mentioned this pull request Dec 15, 2021
73 tasks
@sarabala1979 sarabala1979 mentioned this pull request Mar 1, 2022
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.

Possibility to search archived workflows by name
2 participants