Skip to content

Conversation

@ajanikow
Copy link
Collaborator

@ajanikow ajanikow commented Feb 3, 2022

No description provided.

@cla-bot cla-bot bot added the cla-signed label Feb 3, 2022
@ajanikow ajanikow force-pushed the cleanup/reorganize_reconciliation_context branch from b9bad9a to de0c495 Compare February 3, 2022 08:21
Copy link
Contributor

@informalict informalict left a comment

Choose a reason for hiding this comment

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

LGTM with small comment


// Mode returns the mode of the deployment.
func (d *Deployment) Mode() api.DeploymentMode {
// GetAgencyClientsWithPredicate returns the mode of the deployment.
Copy link
Contributor

Choose a reason for hiding this comment

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

GetMode instead GetAgencyClientsWithPredicate


// Mode returns the mode of the deployment.
func (d *Deployment) Mode() api.DeploymentMode {
// GetAgencyClientsWithPredicate returns the mode of the deployment.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there should be GetMode instead

Copy link
Contributor

@nikita-vanyasin nikita-vanyasin left a comment

Choose a reason for hiding this comment

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

All LGTM except small comment fix for GetMode() function.

These new TODO comments in tests at 235fc6f789315bea8d9d18981b399a7c880bac8cc38421cfba0a91a818e96656R97
also does look a bit confusing but I guess this is some old code moved from one place to another?

@ajanikow
Copy link
Collaborator Author

ajanikow commented Feb 3, 2022

All LGTM except small comment fix for GetMode() function.

These new TODO comments in tests at 235fc6f789315bea8d9d18981b399a7c880bac8cc38421cfba0a91a818e96656R97 also does look a bit confusing but I guess this is some old code moved from one place to another?

We are not implementing not-used test functions - it is desired to do panic with them to make sure that they are not used.

@ajanikow ajanikow force-pushed the cleanup/reorganize_reconciliation_context branch from de0c495 to ec3c005 Compare February 3, 2022 09:20
@ajanikow ajanikow merged commit 58b2ff9 into master Feb 3, 2022
@ajanikow ajanikow deleted the cleanup/reorganize_reconciliation_context branch February 3, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants