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

clean up feature allocation since its always true now #763

Merged
merged 4 commits into from
Jul 11, 2024
Merged

clean up feature allocation since its always true now #763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c890c9c
clean up feature allocation since its always true now
smonero Jul 10, 2024
ba001b6
fix tests
smonero Jul 10, 2024
161b7b0
fix tests
smonero Jul 10, 2024
c1eaee9
fix tests
smonero Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 0 additions & 59 deletions server/legacy/events/policy_filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,6 @@ func (p *ApprovedPolicyFilter) Filter(ctx context.Context, installationToken int
return failedPolicies, nil
}

// Dismiss PR reviews when event came from pull request change/atlantis plan comment
if trigger == command.AutoTrigger || trigger == command.CommentTrigger {
err := p.dismissStalePRReviews(ctx, installationToken, repo, prNum)
if err != nil {
return failedPolicies, errors.Wrap(err, "failed to dismiss stale PR reviews")
}
return failedPolicies, nil
}

// Fetch reviewers who approved the PR
approvedReviewers, err := p.prReviewFetcher.ListLatestApprovalUsernames(ctx, installationToken, repo, prNum)
if err != nil {
Expand All @@ -87,56 +78,6 @@ func (p *ApprovedPolicyFilter) Filter(ctx context.Context, installationToken int
return filteredFailedPolicies, nil
}

func (p *ApprovedPolicyFilter) dismissStalePRReviews(ctx context.Context, installationToken int64, repo models.Repo, prNum int) error {
shouldAllocate, err := p.allocator.ShouldAllocate(feature.LegacyDeprecation, feature.FeatureContext{
RepoName: repo.FullName,
})
if err != nil {
return errors.Wrap(err, "unable to allocate legacy deprecation feature flag")
}
// if legacy deprecation is enabled, don't dismiss stale PR reviews in legacy workflow
if shouldAllocate {
p.logger.InfoContext(ctx, "legacy deprecation feature flag enabled, not dismissing stale PR reviews")
return nil
}

approvalReviews, err := p.prReviewFetcher.ListApprovalReviews(ctx, installationToken, repo, prNum)
if err != nil {
return errors.Wrap(err, "failed to fetch GH PR reviews")
}

for _, approval := range approvalReviews {
isOwner, err := p.approverIsOwner(ctx, installationToken, approval)
if err != nil {
return errors.Wrap(err, "failed to validate approver is owner")
}
if isOwner {
err = p.prReviewDismisser.Dismiss(ctx, installationToken, repo, prNum, approval.GetID())
if err != nil {
return errors.Wrap(err, "failed to dismiss GH PR reviews")
}
}
}
return nil
}

func (p *ApprovedPolicyFilter) approverIsOwner(ctx context.Context, installationToken int64, approval *gh.PullRequestReview) (bool, error) {
if approval.GetUser() == nil {
return false, errors.New("failed to identify approver")
}
reviewers := []string{approval.GetUser().GetLogin()}
for _, policy := range p.policies {
isOwner, err := p.reviewersContainsPolicyOwner(ctx, installationToken, reviewers, policy)
if err != nil {
return false, errors.Wrap(err, "validating policy approval")
}
if isOwner {
return true, nil
}
}
return false, nil
}

func (p *ApprovedPolicyFilter) reviewersContainsPolicyOwner(ctx context.Context, installationToken int64, reviewers []string, policy valid.PolicySet) (bool, error) {
// fetch owners from GH team
owners, err := p.teamMemberFetcher.ListTeamMembers(ctx, installationToken, policy.Owner)
Expand Down
129 changes: 0 additions & 129 deletions server/legacy/events/policy_filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,87 +45,6 @@ func TestFilter_Approved(t *testing.T) {
assert.Empty(t, filteredPolicies)
}

func TestFilter_NotApproved(t *testing.T) {
reviewFetcher := &mockReviewFetcher{
reviews: []*github.PullRequestReview{
{
User: &github.User{Login: github.String(ownerA)},
},
{
User: &github.User{Login: github.String(ownerB)},
},
},
}
teamFetcher := &mockTeamMemberFetcher{
members: []string{ownerC},
}
reviewDismisser := &mockReviewDismisser{}
failedPolicies := []valid.PolicySet{
{Name: policyName, Owner: policyOwner},
}

policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{}, failedPolicies, logging.NewNoopCtxLogger(t))
filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.AutoTrigger, failedPolicies)
assert.NoError(t, err)
assert.False(t, reviewFetcher.listUsernamesIsCalled)
assert.True(t, reviewFetcher.listApprovalsIsCalled)
assert.True(t, teamFetcher.isCalled)
assert.False(t, reviewDismisser.isCalled)
assert.Equal(t, failedPolicies, filteredPolicies)
}

func TestFilter_DismissalBlockedByFeatureAllocator(t *testing.T) {
reviewFetcher := &mockReviewFetcher{
reviews: []*github.PullRequestReview{
{
User: &github.User{Login: github.String(ownerA)},
},
},
}
teamFetcher := &mockTeamMemberFetcher{
members: []string{ownerA},
}
reviewDismisser := &mockReviewDismisser{}
failedPolicies := []valid.PolicySet{
{Name: policyName, Owner: policyOwner},
}

policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{Enabled: true}, failedPolicies, logging.NewNoopCtxLogger(t))
filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.AutoTrigger, failedPolicies)
assert.NoError(t, err)
assert.False(t, reviewFetcher.listUsernamesIsCalled)
assert.False(t, reviewFetcher.listApprovalsIsCalled)
assert.False(t, teamFetcher.isCalled)
assert.False(t, reviewDismisser.isCalled)
assert.Equal(t, failedPolicies, filteredPolicies)
}

func TestFilter_NotApproved_Dismissal(t *testing.T) {
reviewFetcher := &mockReviewFetcher{
reviews: []*github.PullRequestReview{
{
User: &github.User{Login: github.String(ownerA)},
},
},
}
teamFetcher := &mockTeamMemberFetcher{
members: []string{ownerA},
}
reviewDismisser := &mockReviewDismisser{}
failedPolicies := []valid.PolicySet{
{Name: policyName, Owner: policyOwner},
}

policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{}, failedPolicies, logging.NewNoopCtxLogger(t))
filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.AutoTrigger, failedPolicies)
assert.NoError(t, err)
assert.False(t, reviewFetcher.listUsernamesIsCalled)
assert.True(t, reviewFetcher.listApprovalsIsCalled)
assert.True(t, teamFetcher.isCalled)
assert.True(t, reviewDismisser.isCalled)
assert.Equal(t, failedPolicies, filteredPolicies)
}

func TestFilter_NoFailedPolicies(t *testing.T) {
reviewFetcher := &mockReviewFetcher{
approvers: []string{ownerB},
Expand Down Expand Up @@ -166,26 +85,6 @@ func TestFilter_FailedListLatestApprovalUsernames(t *testing.T) {
assert.Equal(t, failedPolicies, filteredPolicies)
}

func TestFilter_FailedListApprovalReviews(t *testing.T) {
reviewFetcher := &mockReviewFetcher{
listApprovalsError: assert.AnError,
}
teamFetcher := &mockTeamMemberFetcher{}
reviewDismisser := &mockReviewDismisser{}
failedPolicies := []valid.PolicySet{
{Name: policyName, Owner: policyOwner},
}

policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{}, failedPolicies, logging.NewNoopCtxLogger(t))
filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.CommentTrigger, failedPolicies)
assert.Error(t, err)
assert.False(t, reviewFetcher.listUsernamesIsCalled)
assert.True(t, reviewFetcher.listApprovalsIsCalled)
assert.False(t, reviewDismisser.isCalled)
assert.False(t, teamFetcher.isCalled)
assert.Equal(t, failedPolicies, filteredPolicies)
}

func TestFilter_FailedTeamMemberFetch(t *testing.T) {
reviewFetcher := &mockReviewFetcher{
approvers: []string{ownerB},
Expand All @@ -208,34 +107,6 @@ func TestFilter_FailedTeamMemberFetch(t *testing.T) {
assert.Equal(t, failedPolicies, filteredPolicies)
}

func TestFilter_FailedDismiss(t *testing.T) {
reviewFetcher := &mockReviewFetcher{
reviews: []*github.PullRequestReview{
{
User: &github.User{Login: github.String(ownerB)},
},
},
}
reviewDismisser := &mockReviewDismisser{
error: assert.AnError,
}
teamFetcher := &mockTeamMemberFetcher{
members: []string{ownerB},
}
failedPolicies := []valid.PolicySet{
{Name: policyName, Owner: policyOwner},
}

policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{}, failedPolicies, logging.NewNoopCtxLogger(t))
filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.AutoTrigger, failedPolicies)
assert.Error(t, err)
assert.False(t, reviewFetcher.listUsernamesIsCalled)
assert.True(t, reviewFetcher.listApprovalsIsCalled)
assert.True(t, teamFetcher.isCalled)
assert.True(t, reviewDismisser.isCalled)
assert.Equal(t, failedPolicies, filteredPolicies)
}

type mockReviewFetcher struct {
approvers []string
listUsernamesIsCalled bool
Expand Down
31 changes: 10 additions & 21 deletions server/legacy/events/pr_project_context_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package events

import (
"fmt"

"github.com/runatlantis/atlantis/server/config/valid"
"github.com/runatlantis/atlantis/server/legacy/events/command"
"github.com/runatlantis/atlantis/server/logging"
Expand Down Expand Up @@ -38,23 +36,14 @@ func (p *PlatformModeProjectContextBuilder) BuildProjectContext(
repoDir string,
contextFlags *command.ContextFlags,
) []command.ProjectContext {
shouldAllocate, err := p.allocator.ShouldAllocate(feature.PlatformMode, feature.FeatureContext{RepoName: ctx.HeadRepo.FullName})
if err != nil {
p.Logger.ErrorContext(ctx.RequestCtx, fmt.Sprintf("unable to allocate for feature: %s, error: %s", feature.PlatformMode, err))
}

if shouldAllocate {
return buildContext(
ctx,
cmdName,
getSteps(cmdName, prjCfg.PullRequestWorkflow, contextFlags.LogLevel),
p.CommentBuilder,
prjCfg,
commentArgs,
repoDir,
contextFlags,
)
}

return p.delegate.BuildProjectContext(ctx, cmdName, prjCfg, commentArgs, repoDir, contextFlags)
return buildContext(
ctx,
cmdName,
getSteps(cmdName, prjCfg.PullRequestWorkflow, contextFlags.LogLevel),
p.CommentBuilder,
prjCfg,
commentArgs,
repoDir,
contextFlags,
)
}
Loading
Loading