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

Fix to use target dsp in script run rollback #5215

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

ffjlabo
Copy link
Member

@ffjlabo ffjlabo commented Sep 17, 2024

What this PR does / why we need it:

Fixed to use TargetDSP on the SCRIPT_RUN_ROLLBACK stage.
With these fixes, we can refer to the files located in the app dir when executing the SCRIPT_RUN_ROLLBACK stage.

I chose TargetDSP, not RunningDSP, because the commands on the onRollback are set in app.pipecd.yaml used when Deployment is executed.
The purpose of the SCRIPT_RUN_ROLLBACK stage is to run whatever command is currently set in onRollback during rollback.

Which issue(s) this PR fixes:

Part of #5214

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

@@ -90,7 +90,7 @@ func (p *provider) Get(ctx context.Context, lw io.Writer) (*DeploySource, error)

if !p.done {
p.source, p.err = p.prepare(ctx, lw)
p.done = true
p.done = p.err == nil // If there is an error, we should re-prepare it next time.
Copy link
Member Author

@ffjlabo ffjlabo Sep 17, 2024

Choose a reason for hiding this comment

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

📝 This modification is necessary to avoid an error when executing a cancellation while executing TargetDSP.Get in another stage.

I encountered such a situation when canceling the deployment via WebUI during the SCRIPT_RUN stage execution like this↓.

Cursor_と_PipeCD__1__png

Copy link
Member Author

Choose a reason for hiding this comment

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

I split the fixes to the another PR #5216

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 22.82%. Comparing base (5d3e17d) to head (8ffadca).

Files with missing lines Patch % Lines
pkg/app/piped/executor/kubernetes/rollback.go 0.00% 7 Missing ⚠️
pkg/app/piped/deploysource/deploysource.go 0.00% 1 Missing ⚠️
pkg/app/pipedv1/deploysource/deploysource.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5215   +/-   ##
=======================================
  Coverage   22.82%   22.82%           
=======================================
  Files         420      420           
  Lines       45302    45309    +7     
=======================================
+ Hits        10340    10344    +4     
- Misses      34166    34170    +4     
+ Partials      796      795    -1     

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

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.

👍🏻

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

@ffjlabo ffjlabo merged commit 0e7a177 into master Sep 19, 2024
16 of 17 checks passed
@ffjlabo ffjlabo deleted the fix-to-use-target-dsp-in-script-run-rollback branch September 19, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants