Skip to content

[Stage Plugins] Use METADATA_STAGE_DISPLAY_KEY instead of SkippedBy/ApprovedBy#5980

Merged
t-kikuc merged 5 commits into
masterfrom
plugin/command/metadata-key-display
Jul 2, 2025
Merged

[Stage Plugins] Use METADATA_STAGE_DISPLAY_KEY instead of SkippedBy/ApprovedBy#5980
t-kikuc merged 5 commits into
masterfrom
plugin/command/metadata-key-display

Conversation

@t-kikuc
Copy link
Copy Markdown
Member

@t-kikuc t-kikuc commented Jul 1, 2025

What this PR does:

  • Show the stage metadata by the common key MetadataKey_StageDisplay instead of SkippedBy and ApprovedBy
    • This will be used in k8s plugin too for the trafficPercentage.

Why we need it:

  • To suppress hard coding of SkippedBy and ApprovedBy.
    • It reduces tightly-coupled code of web/server/piped/plugin

TODO:

  • In the SDK, add pkg/plugin/sdk/constants/constants.go with MetadataKey_StageDisplay
    usage:
import (
   constants "github.com/pipe-cd/piped-plugin-sdk-go/constants"
)

func executeStage(...){
 ...
 client.PutStageMetadata(ctx, constants.MetadataKey_StageDisplay, "Skipped by: user-A")
 ...
}

Which issue(s) this PR fixes:

Fixes Part of #5367

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):

t-kikuc added 2 commits July 1, 2025 09:54
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.16%. Comparing base (b7555ad) to head (831dee3).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5980      +/-   ##
==========================================
- Coverage   28.23%   28.16%   -0.07%     
==========================================
  Files         512      516       +4     
  Lines       55326    55895     +569     
==========================================
+ Hits        15620    15745     +125     
- Misses      38461    38898     +437     
- Partials     1245     1252       +7     
Flag Coverage Δ
. 23.22% <ø> (+<0.01%) ⬆️
.-pkg-app-pipedv1-plugin-kubernetes 66.42% <ø> (ø)
.-pkg-app-pipedv1-plugin-kubernetes_multicluster 67.51% <ø> (ø)
.-pkg-app-pipedv1-plugin-wait 35.51% <ø> (ø)
.-pkg-plugin-sdk 50.89% <ø> (ø)
.-tool-actions-gh-release 19.23% <ø> (ø)
.-tool-actions-plan-preview 25.30% <ø> (?)
.-tool-codegen-protoc-gen-auth 0.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

t-kikuc added 2 commits July 1, 2025 10:12
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Comment thread pkg/constants/constants.go Outdated
Comment on lines +17 to +20
const (
// MetadataKeyStageDisplay is the key of the metadata to be displayed on the deployment detail UI.
MetadataKeyStageDisplay = "pipecd/stage-display-metadata"
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[IMO] How about defining the const value in the model package to clarify when to use it by grouping related things.
For example, Maybe in pkg/model/stage.go?

BTW, I referred the current implementation.
e.g.
https://github.com/pipe-cd/pipecd/blob/master/pkg/model/deployment.go#L23-L25

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it seems good.

Now we don't need to create constants.go.
(I guess MetadataKeyDeploymentNotification will not be used in pipedv1)

tbh, previously the file contained a const for Approvers, which will be used in server too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ffjlabo
I moved: 831dee3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@t-kikuc
I got it, thanks!

If this is the stage-specific knowledge, then we might put it in pkg/model/stage.go.
WDYT?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ffjlabo Oh, I missed stage.go.

But I noticed that stage.go does not seem to be used in pipedv1.
WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, so let's keep it as is.

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@t-kikuc t-kikuc enabled auto-merge (squash) July 2, 2025 06:23
Copy link
Copy Markdown
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@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

@t-kikuc t-kikuc merged commit e43a5a8 into master Jul 2, 2025
37 of 38 checks passed
@t-kikuc t-kikuc deleted the plugin/command/metadata-key-display branch July 2, 2025 06:50
@github-actions github-actions Bot mentioned this pull request Jul 14, 2025
@github-actions github-actions Bot mentioned this pull request Aug 15, 2025
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.

3 participants