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 null pointer #4102

Merged
merged 2 commits into from
Jan 5, 2023
Merged

fix null pointer #4102

merged 2 commits into from
Jan 5, 2023

Conversation

TonkyH
Copy link
Contributor

@TonkyH TonkyH commented Jan 5, 2023

What this PR does / why we need it:
The null pointer was causing the error propagation to not work.
So I've corrected it.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

@khanhtc1202
Copy link
Member

Not related to this PR changes but you should update this
https://github.com/pipe-cd/pipecd/pull/4102/files#diff-d72df5132a62e1bc9918e8313b516291c54b87a84acd3686bf5233adf4775cccR156
pass *taskDefinition.TaskDefinitionArn instead of Family there.

@@ -170,7 +170,7 @@ func (c *client) RunTask(ctx context.Context, taskDefinition types.TaskDefinitio

_, err := c.ecsClient.RunTask(ctx, input)
if err != nil {
return fmt.Errorf("failed to run ECS task %s: %w", *taskDefinition.TaskDefinitionArn, err)
return fmt.Errorf("failed to run ECS task %s: %w", *taskDefinition.Family, err)
Copy link
Member

Choose a reason for hiding this comment

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

Should keep it as is and add a check for TaskDefinicationArn before running task since it doesn't make sense to run a task that does not exist. Please refer to

if taskDefinition.TaskDefinitionArn == nil {
return nil, fmt.Errorf("failed to create task set of task family %s: no task definition provided", *taskDefinition.Family)
}

@TonkyH TonkyH marked this pull request as ready for review January 5, 2023 05:46
Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

Here you go 👍

@khanhtc1202 khanhtc1202 enabled auto-merge (squash) January 5, 2023 06:59
@khanhtc1202 khanhtc1202 merged commit 9d4b72d into pipe-cd:master Jan 5, 2023
This was referenced Jan 6, 2023
knanao pushed a commit that referenced this pull request Jan 13, 2023
* fix null pointer

* apply review
khanhtc1202 added a commit that referenced this pull request Jan 13, 2023
… (#4128)

* Keep new line at end of file in yamlprocessor (#4033)

* Keep new line at end of file in yamlprocessor

* add a new line to the EOF in case of there is no  at EOF

* update comment

* update eventwatcher_test

* consider new line at EOF on parsing kubernetes manifest

* update go-yaml v1.9.8

* remove TODO because PR of go-yaml was merged

* consider to handle multiple new line at EOF

* remove indent vioration of tab

* support standalone ECS task (#4084)

Fixes #2734

* Small lint fix (#4097)

* fix null pointer (#4102)

* fix null pointer

* apply review

* Add example for ecs standalone task (#4104)

* add example for ecs standalone task

* add example for ecs standalone task

* wip

* add yaml example to docs.

* add example for ecs standalone task

* wip

* add yaml example to docs.

* apply review

* Update docs/content/en/docs-dev/user-guide/managing-application/defining-app-configuration/ecs.md

Co-authored-by: Khanh Tran <[email protected]>

Co-authored-by: Khanh Tran <[email protected]>

* Add update contributions command (#4114)

* Add update contributions command

* Update hack/gen-contributions.sh

* Run update go deps (#4117)

* feature: cli and grpc to disable application (#4119)

* Add new line in detailsFormat to fix plan preview format (#4122)

* add document how to disable (#4123)

Co-authored-by: Kurochan <[email protected]>
Co-authored-by: Tomoki Hori <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>
Co-authored-by: kevin55156 <[email protected]>
Co-authored-by: Yoshiaki Ishihara <[email protected]>
This was referenced Feb 7, 2023
@github-actions github-actions bot mentioned this pull request Feb 27, 2023
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.

2 participants