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

Add an ability to filter apps by labels #2987

Merged
merged 2 commits into from
Jan 5, 2022
Merged

Conversation

nakabonne
Copy link
Member

What this PR does / why we need it:
Aside from AutocompleteI couldn't find out any material UI component which accepts multiple texts that look like those mentioned in the issue.

You can't select other than suggested options, that brings us a kind of validation for label format.

filtering-apps-by-labels.mov

Which issue(s) this PR fixes:

Fixes #2756
Ref #2978

Does this PR introduce a user-facing change?:

Filtering applications by Labels is now available

@nakabonne
Copy link
Member Author

This is ready to be reviewed, but it must not get shipped until the next version will be out.
/hold

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 31.59%. This pull request does not change code coverage.

@nghialv
Copy link
Member

nghialv commented Dec 24, 2021

This is ready to be reviewed, but it must not get shipped until the next version will be out.

I think we don't have to wait for that time. This can be normally included in the next release (v0.23.0).

The idea is to break the "filtering by label" feature into smaller ones:
"support filtering by labels for the Applications page"
and
"support for the Deployments page"

This PR is for the first one and will be included in v0.23.0.
The second one will be scheduled later.

@nakabonne
Copy link
Member Author

@nghialv Technically we can 👍
I was just worried about the next version will be way too feature-packed. Including this filtering feature means including the Labels concept as well. If you are fine, I will get started preparing documentations for Labels concept.

@nghialv
Copy link
Member

nghialv commented Dec 24, 2021

@nakabonne Nice point!
What kind of doc do you intend to have for the Label feature?

@nakabonne
Copy link
Member Author

Pages needed to be updated are:

  • concepts/ - add Labels description
  • user-guide/adding-an-application/ - add an example of setting labels, and mention that Labels can be set only via an app config file.
  • Everywhere mentioning Environment- mark them deprecated

Seeing it actually, there is not that much to deal with.

@nakabonne
Copy link
Member Author

Well, either way, it's fine to keep this PR hold if we can release v0.23.0 as soon as the new year starts 👍

@nghialv
Copy link
Member

nghialv commented Dec 24, 2021

Well, either way, it's fine to keep this PR hold if we can release v0.23.0 as soon as the new year starts 👍

Yea. I'm fine with that way.

@nghialv
Copy link
Member

nghialv commented Dec 24, 2021

concepts/ - add Labels description

nit: Label is an obvious one so maybe we don't need its definition.

@nakabonne
Copy link
Member Author

/rebase

@pipecd-bot
Copy link
Collaborator

REBASE

@nakabonne: Rebased this pull request in response to this comment.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 31.57%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 82.35%. This pull request decreases coverage by -0.27%.

File Base Head Diff
src/api/applications.ts 84.40% 83.78% -0.62%
src/components/applications-page/application-filter/index.tsx 100.00% 76.09% -23.91%
src/modules/applications/index.ts 83.13% 80.90% -2.23%
src/utils/search-params.ts 70.00% 72.73% +2.73%

@nakabonne
Copy link
Member Author

/rebase

@pipecd-bot
Copy link
Collaborator

REBASE

@nakabonne: Rebased this pull request in response to this comment.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 31.57%. This pull request does not change code coverage.

@nakabonne
Copy link
Member Author

/trigger web-test

@pipecd-bot
Copy link
Collaborator

TRIGGER

@nakabonne: Your requested web-test has been scheduled in response to this comment.

@khanhtc1202
Copy link
Member

khanhtc1202 commented Jan 4, 2022

Nice work, just one point I worried about. There is no way to make users know the format of labels they should use, so how about add things like this for Labels field

Screen Shot 2022-01-04 at 13 34 28

I mean, the title field is remained as Labels as you did, but for the placeholder, we can use text like in format key:value so that users could aware of what they should write there as a label, wdyt. Currently, the placeholder value is Labels which does not help very much
https://github.com/pipe-cd/pipe/pull/2987/files#diff-a4338d3a1daca78bef95b4f629ea59f0bc27b92d830427d34da256960e501e17R231

@nakabonne
Copy link
Member Author

@khanhtc1202 Good idea! That placeholder is definitely more valuable!

@nakabonne
Copy link
Member Author

@khanhtc1202 Applied your idea

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 31.57%. This pull request does not change code coverage.

@nakabonne
Copy link
Member Author

/trigger web-test

@pipecd-bot
Copy link
Collaborator

TRIGGER

@nakabonne: Your requested web-test has been scheduled in response to this comment.

@khanhtc1202
Copy link
Member

Nice work 👍
/lgtm

@pipecd-bot pipecd-bot added the lgtm label Jan 4, 2022
@nakabonne
Copy link
Member Author

/trigger web-test

@pipecd-bot
Copy link
Collaborator

TRIGGER

@nakabonne: Your requested web-test has been scheduled in response to this comment.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 82.35%. This pull request decreases coverage by -0.28%.

File Base Head Diff
src/components/applications-page/application-filter/index.tsx 100.00% 76.09% -23.91%
src/modules/applications/index.ts 83.13% 80.90% -2.23%
src/api/applications.ts 84.40% 83.78% -0.62%

@nakabonne
Copy link
Member Author

https://github.com/pipe-cd/pipe/releases/tag/v0.23.0 got shipped which made this mergeable.
/hold cancel

@nghialv
Copy link
Member

nghialv commented Jan 4, 2022

Great!
/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

@khanhtc1202
Copy link
Member

/trigger web-test

@pipecd-bot
Copy link
Collaborator

TRIGGER

@khanhtc1202: Your requested web-test has been scheduled in response to this comment.

@nakabonne
Copy link
Member Author

/trigger web-test

@pipecd-bot
Copy link
Collaborator

TRIGGER

@nakabonne: Your requested web-test has been scheduled in response to this comment.

@nakabonne
Copy link
Member Author

/trigger web-test

@pipecd-bot
Copy link
Collaborator

TRIGGER

@nakabonne: Your requested web-test has been scheduled in response to this comment.

@nakabonne
Copy link
Member Author

It's rare for web-test to succeed anymore 😓

@nakabonne
Copy link
Member Author

/trigger web-test

@pipecd-bot
Copy link
Collaborator

TRIGGER

@nakabonne: Your requested web-test has been scheduled in response to this comment.

@pipecd-bot pipecd-bot merged commit d2fb908 into master Jan 5, 2022
@pipecd-bot pipecd-bot deleted the feature-label-filtering branch January 5, 2022 02:20
pipecd-bot pushed a commit that referenced this pull request Jan 6, 2022
**What this PR does / why we need it**:
This PR adds an ability to filter deployments by Labels like [the applications page](#2987).
Keep in mind the GIF animation below is what merges the MORE button PR (#2999).

![Kapture 2022-01-05 at 15 17 06](https://user-images.githubusercontent.com/19730728/148169587-7ee2c3ab-da0c-4d65-843a-9cfb13899f44.gif)


**Which issue(s) this PR fixes**:

Fixes #2978
Fixes #2757

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
-->
```release-note
Filtering deployments by Labels is now available
```

This PR was merged by Kapetanios.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify labels for ListApplications
4 participants