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.

Sign up for GitHub

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

Jump to bottom

Log ensureArtifact ConflictErr #19294

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Log ensureArtifact ConflictErr #19294

merged 5 commits into from
Jan 15, 2024

Conversation

LiuShuaiyi
Copy link
Contributor

@LiuShuaiyi LiuShuaiyi commented Sep 1, 2023 •
edited by Vad1mo
Loading

Thank you for contributing to Harbor!

Comprehensive Summary of your change

https://github.com/goharbor/harbor/blob/main/src/controller/artifact/controller.go#L230
conflict error will always be overwritten here if GetByDigest returns an error, making it hard to debug the conflict error. This change logs the conflict error before it gets overwritten

Issue being fixed

Fixes #19293

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • [ x] Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • [v] Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@LiuShuaiyi LiuShuaiyi requested a review from a team as a code owner September 1, 2023 23:25
@LiuShuaiyi
Copy link
Contributor Author

Friendly nudge for review : )

@codecov
Copy link

codecov bot commented Sep 8, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@fdc012c). Click here to learn what that means.

❗ Current head 5bff359 differs from pull request most recent head d87cdf5. Consider uploading reports for the commit d87cdf5 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #19294   +/-   ##
=======================================
  Coverage        ?   67.56%           
=======================================
  Files           ?      991           
  Lines           ?   109172           
  Branches        ?     2720           
=======================================
  Hits            ?    73757           
  Misses          ?    31449           
  Partials        ?     3966
Flag Coverage Δ
unittests 67.56% <100.00%> (?)

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

Files Coverage Δ
src/controller/artifact/controller.go 63.87% <100.00%> (ø)

chlins
chlins reviewed Sep 8, 2023
View reviewed changes
src/controller/artifact/controller.go Outdated
@@ -227,6 +227,7 @@ func (c *controller) ensureArtifact(ctx context.Context, repository, digest stri
if !errors.IsConflictErr(err) {
return false, nil, err
}
log.Debugf("failed to create artifact %d: %v", art.ID, err)
Copy link
Member

@chlins chlins Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The var art is assigned in the first line of this function, it should be nil if the artifact is not found, so here print art.ID may cause the panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch! I also added a test case in the unit test for this code path.

@LiuShuaiyi
Copy link
Contributor Author

Gentle nudge for review. Thanks!

@LiuShuaiyi
Copy link
Contributor Author

Ping for review. Thanks

chlins
chlins approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@chlins chlins left a comment

Choose a reason for hiding this comment

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

lgtm

@chlins chlins added the release-note/update Update or Fix label Oct 13, 2023
wy65701436
wy65701436 approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

@wy65701436 wy65701436 merged commit f17d90f into goharbor:main Jan 15, 2024
10 checks passed
altynbaev pushed a commit to altynbaev/harbor that referenced this pull request Jan 29, 2024
@LiuShuaiyi @wy65701436
Log ensureArtifact ConflictErr (goharbor#19294)
2eb51ae 
* Log ensureArtifact ConflictErr

Signed-off-by: Shuaiyi Liu <[email protected]>

* Log ensureArtifact ConflictErr

Signed-off-by: Shuaiyi Liu <[email protected]>

---------

Signed-off-by: Shuaiyi Liu <[email protected]>
Co-authored-by: Wang Yan <[email protected]>
Signed-off-by: Altynbaev Dinislam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chlins chlins chlins approved these changes

@wy65701436 wy65701436 wy65701436 approved these changes

Assignees

@wy65701436 wy65701436

@chlins chlins

@MinerYang MinerYang

Labels
release-note/update Update or Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Harbor doesn't log the conflict error in ensureArtifact
4 participants
@LiuShuaiyi @wy65701436 @chlins @MinerYang