Skip to content

Comments

feat: add svc export trackability #1061

Merged
ryanzhang-oss merged 6 commits intoAzure:mainfrom
britaniar:svcExportTrackability
Mar 13, 2025
Merged

feat: add svc export trackability #1061
ryanzhang-oss merged 6 commits intoAzure:mainfrom
britaniar:svcExportTrackability

Conversation

@britaniar
Copy link
Contributor

@britaniar britaniar commented Feb 28, 2025

Description of your changes

Fixes #

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

@britaniar britaniar force-pushed the svcExportTrackability branch 5 times, most recently from 87efd16 to e49aff2 Compare March 4, 2025 19:25
@britaniar britaniar marked this pull request as ready for review March 4, 2025 22:07
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss left a comment

Choose a reason for hiding this comment

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

this should be separated into two PRs, one just for upgrade controller runtime and the other for serviceExport check

@britaniar
Copy link
Contributor Author

britaniar commented Mar 5, 2025

upgrade controller runtime changes are addressed in #1067 (will need to be merged before this)

@britaniar britaniar force-pushed the svcExportTrackability branch from 9b3574b to 593b6b3 Compare March 5, 2025 01:01
@britaniar britaniar force-pushed the svcExportTrackability branch from 593b6b3 to c47b06c Compare March 5, 2025 22:30
@britaniar britaniar force-pushed the svcExportTrackability branch from 74d71eb to 6b43ca2 Compare March 11, 2025 18:09
@ryanzhang-oss ryanzhang-oss merged commit 22dc1a0 into Azure:main Mar 13, 2025
16 checks passed
if err != nil {
klog.V(2).InfoS("ServiceExport has invalid weight", "serviceExport", svcExportObj, "error", err)
return ManifestProcessingAvailabilityResultTypeFailed, err
klog.Errorf(err.Error(), "ServiceExport has invalid weight", "serviceExport", svcExportObj)
Copy link
Contributor

Choose a reason for hiding this comment

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

klog.Errorf(err, "ServiceExport has invalid weight", "serviceExport", svcExportObj)

you don't need to call the err.Error(). the klog will print out the error message automatically.

return ManifestProcessingAvailabilityResultTypeNotYetAvailable, nil
}

// Validate annotation weight. Updating the annotation won't change the object generation,
Copy link
Contributor

Choose a reason for hiding this comment

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

missing period.

klog.V(2).InfoS("ServiceExport has invalid weight", "serviceExport", svcExportObj, "error", err)
return ManifestProcessingAvailabilityResultTypeFailed, err
klog.Errorf(err.Error(), "ServiceExport has invalid weight", "serviceExport", svcExportObj)
return ManifestProcessingAvailabilityResultTypeNotYetAvailable, err
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not return the error here. no need to retry.
Sorry, it was missed before :(

@britaniar britaniar deleted the svcExportTrackability branch March 13, 2025 16:49
zhiying-lin added a commit to zhiying-lin/fleet that referenced this pull request Mar 20, 2025
ryanzhang-oss pushed a commit that referenced this pull request Mar 20, 2025
* Revert "address comments to fix bug (#1080)"

This reverts commit 9a376db.

* Revert "feat: add svc export trackability  (#1061)"

This reverts commit 22dc1a0.
ryanzhang-oss pushed a commit to ryanzhang-oss/fleet that referenced this pull request Apr 22, 2025
ryanzhang-oss pushed a commit to ryanzhang-oss/fleet that referenced this pull request Apr 22, 2025
* Revert "address comments to fix bug (Azure#1080)"

This reverts commit 9a376db.

* Revert "feat: add svc export trackability  (Azure#1061)"

This reverts commit 22dc1a0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants