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(manifest-replicas): ReplicaSet Source-Capacity #5874

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

nhtzr
Copy link
Contributor

@nhtzr nhtzr commented Feb 10, 2023

Change: Use version suffixes to populate replicas when using use-source-capacity annotation

@nhtzr
Copy link
Contributor Author

nhtzr commented Feb 13, 2023

@Mergifyio backport release-1.28.x release-1.27.x

@mergify
Copy link
Contributor

mergify bot commented Feb 13, 2023

backport release-1.28.x release-1.27.x

❌ Command disallowed due to command restrictions in the Mergify configuration.

  • sender-permission>=write

@mattgogerly
Copy link
Member

Can you explain what's currently broken and how this fixes it?

@nhtzr
Copy link
Contributor Author

nhtzr commented Feb 14, 2023

@mattgogerly use-source-capacity annotation is meant to query the currently used replicas field from the manifest in the cluster. The behavior seems to run fine with non-versioned manifests. However, the current implementation fails to take into account the versioning that spinnaker usually adds to ReplicaSets. The fix introduces extra to logic to query the replicas field from the correct manifest name.

@mrampton
Copy link
Contributor

@nhtzr -- would you mind adding some tests?

@nhtzr
Copy link
Contributor Author

nhtzr commented Feb 16, 2023

@mrampton updated

@nhtzr
Copy link
Contributor Author

nhtzr commented Feb 21, 2023

@spinnaker/reviewers Hi, please let me know if I'm missing anything.

@nhtzr
Copy link
Contributor Author

nhtzr commented Feb 21, 2023

@ovidiupopa07 Hi, please let me know if I'm missing anything.

Copy link
Contributor

@ovidiupopa07 ovidiupopa07 left a comment

Choose a reason for hiding this comment

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

LGTM. Would it make sense to also add an IT for this? (there are already ITs written for the Deploy manifest operation)

@nhtzr
Copy link
Contributor Author

nhtzr commented Feb 22, 2023

Added Integration tests for both use-source-capacity cases

Copy link
Contributor

@ovidiupopa07 ovidiupopa07 left a comment

Choose a reason for hiding this comment

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

Lgtm

@ovidiupopa07 ovidiupopa07 added the ready to merge Approved and ready for a merge label Feb 23, 2023
@mergify mergify bot added the auto merged Merged automatically by a bot label Feb 23, 2023
@ovidiupopa07
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2023

update

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
@armory needs to authorize modification on its head branch.
err-code: D0DED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for a merge target-release/1.30
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants