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(archive): Add support to filter list by labels. Closes #2171 #2205

Merged
merged 21 commits into from
Feb 17, 2020
Merged

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Feb 10, 2020

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
  • Optional. I've added My organization is added to the README.
  • I've signed the CLA and required builds are green.

Closes #2171

@alexec alexec changed the title Labels feat(archive): Add support to filter list by labels. Closes #2171 Feb 10, 2020
@alexec alexec added this to the v2.6 milestone Feb 12, 2020
@alexec alexec marked this pull request as ready for review February 12, 2020 22:31
@alexec
Copy link
Contributor Author

alexec commented Feb 12, 2020

@sarabala1979 @whynowy could you review this please? It would be good to get into v2.5.

persist/sqldb/workflow_archive.go Outdated Show resolved Hide resolved
persist/sqldb/db_type.go Outdated Show resolved Hide resolved
persist/sqldb/db_type.go Outdated Show resolved Hide resolved
Select("name", "namespace", "uid", "phase", "startedat", "finishedat").
From(archiveTableName).
Where(db.Cond{"clustername": r.clusterName}).
And(namespaceEqual(namespace)).
And(clause).
Copy link
Member

Choose a reason for hiding this comment

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

Curious about the performance if there are 10K, 100K archived workflows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should consider testing this

}

func (r *workflowArchive) ListWorkflows(namespace string, limit int, offset int) (wfv1.Workflows, error) {
func (r *workflowArchive) ListWorkflows(namespace string, labelRequirements labels.Requirements, limit int, offset int) (wfv1.Workflows, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Labels key-values need to be set in the returned workflow list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Member

@whynowy whynowy Feb 14, 2020

Choose a reason for hiding this comment

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

Those workflow labels will be used to populate label suggestions in the filter component, considering it's already paginated, having them shouldn't bring much performance concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how we'd do this, joining onto argo_archived_workflow_label would be a cartesian join, and selecting from the workflow column would negatively impact performance.

Unless this is a must-have - I don't think we can do it.

@alexec alexec merged commit d309d5c into master Feb 17, 2020
@alexec alexec deleted the labels branch February 17, 2020 20: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.

Listing archived workflows ignores label selector
3 participants