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: check err before use schedule and duration #20043

Merged
merged 5 commits into from
Oct 12, 2024

Conversation

daengdaengLee
Copy link
Member

@daengdaengLee daengdaengLee commented Sep 21, 2024

Fix #19605

I have made some modifications to the related code, though it doesn't fully resolve the issue.
I have made some modifications to resolve the issue.
Previously, schedule and duration were used before checking sErr and dErr, which could result in a nil pointer exception if an error occurred. (use values in line 2433, check error in line 2435)
I have updated the code to check for errors first and use continue if an error is found.

If I've misunderstood something, please let me know in the comments. Thank you! :)

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@daengdaengLee daengdaengLee requested a review from a team as a code owner September 21, 2024 08:23
Copy link

bunnyshell bot commented Sep 21, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Copy link

bunnyshell bot commented Sep 21, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Copy link

codecov bot commented Sep 21, 2024

Codecov Report

Attention: Patch coverage is 75.94937% with 19 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@cf498f6). Learn more about missing BASE report.
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
server/application/application.go 36.36% 3 Missing and 4 partials ⚠️
controller/sync.go 69.23% 2 Missing and 2 partials ⚠️
cmd/argocd/commands/app.go 66.66% 0 Missing and 2 partials ⚠️
cmd/argocd/commands/projectwindows.go 0.00% 2 Missing ⚠️
pkg/apis/application/v1alpha1/types.go 95.23% 1 Missing and 1 partial ⚠️
server/project/project.go 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #20043   +/-   ##
=========================================
  Coverage          ?   55.97%           
=========================================
  Files             ?      322           
  Lines             ?    44759           
  Branches          ?        0           
=========================================
  Hits              ?    25052           
  Misses            ?    17103           
  Partials          ?     2604           

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

@blakepettersson
Copy link
Member

Hi @daengdaengLee it looks good so far! Can we just verify that this is actually fixed by adding a test case or two to

func TestSyncWindows_InactiveAllows(t *testing.T) {
?

@daengdaengLee
Copy link
Member Author

Hello, @blakepettersson.

I have added unit tests for the incorrect schedule and duration.
Please check it.

Thank you!

@blakepettersson
Copy link
Member

I noticed in the original issue that there are two other places where this is an issue (search for specParser.Parse in types.go. Could you also handle those ones?

@daengdaengLee
Copy link
Member Author

daengdaengLee commented Sep 23, 2024

Hello, @blakepettersson.

Thanks for the comment.
I was planning to submit the other parts in a separate PR.
But if that's your preference, I'll handle everything in this PR!


I've handled other things too!
Please check the PR.

Thanks!

@daengdaengLee daengdaengLee force-pushed the issue-19605 branch 3 times, most recently from 78d7443 to 3cb6731 Compare September 23, 2024 13:58
Copy link
Member

@blakepettersson blakepettersson left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@daengdaengLee
Copy link
Member Author

Hello, @blakepettersson.

I rebase the PR to the latest master branch.

How can I get a review for the PR?
Please let me know.

Thanks!

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Instead of logging errors in types.go (which is kind of a place for utility functions), could you modify both functions to return an error? That way the consumer can handle the error appropriately, either by logging or by returning the error message to the caller (as in the API Server's Sync method).

@daengdaengLee
Copy link
Member Author

Hello, @crenshaw-dev!

  • return err when sync window is invalid in types.go
  • keep as close to the original behavior as possible where the functions from types.go are used
  • add/edit unit tests for functions in types.go

Please check and comment if you think differently.
Thanks!

Copy link
Member

@crenshaw-dev crenshaw-dev 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!

Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Signed-off-by: daengdaengLee <gunho1020@gmail.com>
@crenshaw-dev crenshaw-dev merged commit 9b11b21 into argoproj:master Oct 12, 2024
27 checks passed
@crenshaw-dev
Copy link
Member

/cherry-pick release-2.13

gcp-cherry-pick-bot bot pushed a commit that referenced this pull request Oct 12, 2024
* fix: check err before use schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* test: add tests for invalid schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* feat: change to return error when sync window is invalid

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use assert.Error or assert.NoError

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use require instead of assert

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

---------

Signed-off-by: daengdaengLee <gunho1020@gmail.com>
@crenshaw-dev
Copy link
Member

/cherry-pick release-2.12

Copy link

Cherry-pick failed with Merge error 9b11b21f00f006ec5bfca1ff39210e54b65bf4b5 into temp-cherry-pick-963c53-release-2.12

@crenshaw-dev
Copy link
Member

@daengdaengLee if you'd like to cherry-pick back to 2.10-2.12, looks that those will require manual PRs.

crenshaw-dev pushed a commit that referenced this pull request Oct 12, 2024
* fix: check err before use schedule and duration



* test: add tests for invalid schedule and duration



* feat: change to return error when sync window is invalid



* fix: use assert.Error or assert.NoError



* fix: use require instead of assert



---------

Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Co-authored-by: Kunho Lee <gunho1020@gmail.com>
@daengdaengLee daengdaengLee deleted the issue-19605 branch October 14, 2024 12:58
daengdaengLee added a commit to daengdaengLee/argo-cd that referenced this pull request Oct 14, 2024
* fix: check err before use schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* test: add tests for invalid schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* feat: change to return error when sync window is invalid

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use assert.Error or assert.NoError

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use require instead of assert

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

---------

Signed-off-by: daengdaengLee <gunho1020@gmail.com>
austin5219 pushed a commit to austin5219/argo-cd that referenced this pull request Oct 16, 2024
* fix: check err before use schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* test: add tests for invalid schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* feat: change to return error when sync window is invalid

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use assert.Error or assert.NoError

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use require instead of assert

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

---------

Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>
adriananeci pushed a commit to adriananeci/argo-cd that referenced this pull request Dec 4, 2024
* fix: check err before use schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* test: add tests for invalid schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* feat: change to return error when sync window is invalid

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use assert.Error or assert.NoError

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use require instead of assert

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

---------

Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Signed-off-by: Adrian Aneci <aneci@adobe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

nil pointer dereference when user provides invalid cron schedule
3 participants