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

Sort results of plan-preview #5540

Merged
merged 7 commits into from
Feb 16, 2025
Merged

Conversation

kj455
Copy link
Contributor

@kj455 kj455 commented Feb 4, 2025

What this PR does:
Adds the sort-key-labels feature to pipectl plan-preview.

The order of plan-preview results:

  • Default: Sorted by PipedId and ApplicationName
  • If specified: Sorted by the given label keys

Why we need it:
A sorted output would improve readability and facilitate easier identification of the modifications being made.

Which issue(s) this PR fixes:

Fixes #5539

Does this PR introduce a user-facing change?:

  • How are users affected by this change:
  • Is this breaking change:
  • How to migrate (if breaking change):

@kj455 kj455 force-pushed the sort-planpreview-results branch from b999433 to 343a5e1 Compare February 4, 2025 23:14
pkg/app/server/grpcapi/api.go Outdated Show resolved Hide resolved
@kj455 kj455 force-pushed the sort-planpreview-results branch from 4615cc4 to 910bf2f Compare February 8, 2025 09:06
@kj455 kj455 marked this pull request as ready for review February 8, 2025 09:06
Copy link
Member

@t-kikuc t-kikuc left a comment

Choose a reason for hiding this comment

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

@kj455
Thank you so much!!
Please fix the lint error as follows.

I'm still checking the logic (basically, your code seems great)

pkg/app/piped/planpreview/builder_test.go Outdated Show resolved Hide resolved
Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
@kj455 kj455 force-pushed the sort-planpreview-results branch from 910bf2f to cd13d3b Compare February 10, 2025 09:25
@kj455 kj455 requested a review from t-kikuc February 10, 2025 09:28
Copy link
Member

@t-kikuc t-kikuc left a comment

Choose a reason for hiding this comment

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

Thank you so much, I'd like to propose moving the code for customizability.

pkg/app/piped/planpreview/builder.go Outdated Show resolved Hide resolved
Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
@kj455 kj455 force-pushed the sort-planpreview-results branch from 8d15fc2 to bc83f74 Compare February 13, 2025 21:28
@kj455 kj455 requested a review from t-kikuc February 13, 2025 21:32
Copy link
Member

@t-kikuc t-kikuc left a comment

Choose a reason for hiding this comment

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

Thank you so much!!! It will work well.

Please add some tests and check if they result as you expect 🙏

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
@kj455 kj455 force-pushed the sort-planpreview-results branch from 1bf47e1 to 14b52df Compare February 14, 2025 07:42
@kj455
Copy link
Contributor Author

kj455 commented Feb 14, 2025

@t-kikuc
Thank you for your review!

I’ve added test cases based on your advice. 😍

14b52df

@kj455 kj455 requested a review from t-kikuc February 14, 2025 07:44
t-kikuc
t-kikuc previously approved these changes Feb 14, 2025
Copy link
Member

@t-kikuc t-kikuc left a comment

Choose a reason for hiding this comment

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

Thank you so much!!!!

(I'm sorry for changing the way again and again 🙏 )

TODO:

  • update docs
  • add options in the GitHub action

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
@kj455
Copy link
Contributor Author

kj455 commented Feb 14, 2025

@t-kikuc
Thank you for your kind review!

I really appreciate your help in shaping the implementation policy for this PR.

update docs

I have updated the documentation:
025c5c7

add options in the GitHub action

I have submitted a PR for this!
pipe-cd/actions-plan-preview#19

@kj455 kj455 requested a review from t-kikuc February 14, 2025 11:21
Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
@kj455 kj455 requested a review from t-kikuc February 14, 2025 11:41
Copy link
Member

@t-kikuc t-kikuc left a comment

Choose a reason for hiding this comment

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

LGreatTM
Thank you so much

Copy link
Contributor

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@Warashi Warashi merged commit a6511aa into pipe-cd:master Feb 16, 2025
15 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 17, 2025
* Sort results of plan-preview

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* Ensure the order of list piped

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: lint

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: move sorting to pipectl

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: add testcase

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: dev docs

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* add docs

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

---------

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>
github-actions bot pushed a commit that referenced this pull request Feb 17, 2025
* Sort results of plan-preview

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* Ensure the order of list piped

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: lint

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: move sorting to pipectl

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: add testcase

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: dev docs

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* add docs

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

---------

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>
t-kikuc added a commit that referenced this pull request Feb 17, 2025
* Correct notification routing for `DEPLOYMENT_STARTED` (#5523)

* Correct notification routing for `DEPLOYMENT_STARTED`

Signed-off-by: Yuki Okushi <okushi@canary-inc.jp>

* Harden test case

Signed-off-by: Yuki Okushi <okushi@canary-inc.jp>

---------

Signed-off-by: Yuki Okushi <okushi@canary-inc.jp>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Sort results of plan-preview (#5540)

* Sort results of plan-preview

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* Ensure the order of list piped

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: lint

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: move sorting to pipectl

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: add testcase

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* fix: dev docs

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

* add docs

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>

---------

Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Enhanced EventWatcher logs (#5558)

* Show push error log earlier than reporting

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Use WarnLog in retry

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* clarify log messages

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* clarify log messages

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* add TestDoCalls for asserting counts

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* add eventIDs in log

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* enrich logs in updateValues

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* nits

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Revert "add TestDoCalls for asserting counts"

This reverts commit de3f112.

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* update RELEASE to v0.50.2 with doc update (#5571)

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

---------

Signed-off-by: Yuki Okushi <okushi@canary-inc.jp>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>
Signed-off-by: kj455 <kaji.ibuki45@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Co-authored-by: Yuki Okushi <okushi@canary-inc.jp>
Co-authored-by: Ibuki Kaji <38521709+kj455@users.noreply.github.com>
Co-authored-by: Tetsuya KIKUCHI <97105818+t-kikuc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Default sorting of the plan-preview output
3 participants