Strictly count deployment approvals by rule
Problem
In 17.0 we deprecated unified approval rules and migrated them all to multi access level rules.
Multi access level rules should each meet the required approval count separately.
Currently they are just summed.
This won't have any negative impact in terms of blocking an deployment, but could potentially be a problem where you wanted specific groups to approve of a deployment, and its able to be unblocked without all the desired approvals.
Code paths
in ee/app/models/ee/environment.rb
def required_approval_count
return 0 unless protected?
return 0 unless has_approval_rules?
associated_approval_rules.sum(&:required_approvals)
end
in ee/app/models/ee/deployment.rb
def pending_approval_count
required_approval_count = environment.required_approval_count
return 0 unless required_approval_count > 0
[required_approval_count - approvals.length, 0].max
end
Need to research this scenario:
- set up 2 groups with you want to require approval from and create approval rules that both have 1 required approval. It will look like this:
[15] deployment.environment.associated_approval_rules
=> [#<ProtectedEnvironments::ApprovalRule:0x0000000174d71188
id: 67,
protected_environment_id: 125,
user_id: nil,
group_id: 354,
created_at: Fri, 01 Mar 2024 19:03:47.572264000 UTC +00:00,
updated_at: Fri, 01 Mar 2024 19:03:47.572264000 UTC +00:00,
access_level: nil,
required_approvals: 1,
group_inheritance_type: 0>,
#<ProtectedEnvironments::ApprovalRule:0x0000000174d71048
id: 68,
protected_environment_id: 125,
user_id: nil,
group_id: 355,
created_at: Fri, 01 Mar 2024 19:03:47.575463000 UTC +00:00,
updated_at: Fri, 01 Mar 2024 19:03:47.575463000 UTC +00:00,
access_level: nil,
required_approvals: 1,
group_inheritance_type: 0>]
Approve the deployment once as a user with nothing in the representedAs
field.
Approve a second time with one of the groups in the representedAs
field
Then check deployment approvals
[17] deployment.approvals
=> [#<Deployments::Approval:0x0000000174cd9ec8
id: 95,
deployment_id: 125,
user_id: 274,
created_at: Fri, 01 Mar 2024 19:03:47.427578000 UTC +00:00,
updated_at: Fri, 01 Mar 2024 19:03:47.427578000 UTC +00:00,
status: "approved",
comment: "Original comment",
approval_rule_id: nil>,
#<Deployments::Approval:0x0000000174cd9d88
id: 96,
deployment_id: 125,
user_id: 274,
created_at: Fri, 01 Mar 2024 19:03:47.728556000 UTC +00:00,
updated_at: Fri, 01 Mar 2024 19:03:47.728556000 UTC +00:00,
status: "approved",
comment: "Original comment",
approval_rule_id: 67>]
As expected, only one of the approval rules is represented here. The other one is just the User with no representedAs
(which translates into finding the group)
Potential problem: Because of the code copied at the top, deployment is not considered waiting for approval.
[18] deployment.waiting_for_approval?
=> false
And trying to approve again as the second group results in the Deployments::ApprovalService
executing this:
return _('This deployment is not waiting for approvals.') unless deployment.waiting_for_approval?
This won't have any negative impact in terms of blocking an deployment, but could potentially be a problem where you wanted specific groups to approve of a deployment, and its able to be unblocked without all the desired approvals.
This seems wrong. I could be misunderstanding something, but I think if there's a an approval rule with a required approval, that has not yet been approved it should still be waiting for approval.
Proposal
- change the logic to return false if the required approvals of each rule are not met