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

Discard approvals by default in v0.22.0 due to missing logic #2911

Closed
nitrocode opened this issue Jan 3, 2023 · 1 comment · Fixed by #2913
Closed

Discard approvals by default in v0.22.0 due to missing logic #2911

nitrocode opened this issue Jan 3, 2023 · 1 comment · Fixed by #2913
Labels
bug Something isn't working

Comments

@nitrocode
Copy link
Member

nitrocode commented Jan 3, 2023

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment.

Overview of the Issue

See #2696 (comment)

The discarding of approvals is default on accident. There isn't any logic to enable or disable it despite the value of the flag

Reproduction Steps

Logs

Environment details

v0.22.0

Additional Context

Workaround is to use v0.21.0

Edit: we need a check here

func (p *PlanCommandRunner) run(ctx *command.Context, cmd *CommentCommand) {
var err error
baseRepo := ctx.Pull.BaseRepo
pull := ctx.Pull
if err = p.pullUpdater.VCSClient.DiscardReviews(baseRepo, pull); err != nil {
ctx.Log.Err("failed to remove approvals: %s", err)
}

Before a check can be added, we need to add a boolean here to PlanCommandRunner

type PlanCommandRunner struct {
vcsClient vcs.Client
// SilenceNoProjects is whether Atlantis should respond to PRs if no projects
// are found
SilenceNoProjects bool
// SilenceVCSStatusNoPlans is whether autoplan should set commit status if no plans
// are found
silenceVCSStatusNoPlans bool
// SilenceVCSStatusNoPlans is whether any plan should set commit status if no projects
// are found
silenceVCSStatusNoProjects bool
commitStatusUpdater CommitStatusUpdater
pendingPlanFinder PendingPlanFinder
workingDir WorkingDir
prjCmdBuilder ProjectPlanCommandBuilder
prjCmdRunner ProjectPlanCommandRunner
dbUpdater *DBUpdater
pullUpdater *PullUpdater
policyCheckCommandRunner *PolicyCheckCommandRunner
autoMerger *AutoMerger
parallelPoolSize int
pullStatusFetcher PullStatusFetcher
lockingLocker locking.Locker

Before doing that, you'd need to pass the flag here

atlantis/server/server.go

Lines 660 to 668 in 9595b23

policyCheckCommandRunner := events.NewPolicyCheckCommandRunner(
dbUpdater,
pullUpdater,
commitStatusUpdater,
instrumentedProjectCmdRunner,
userConfig.ParallelPoolSize,
userConfig.SilenceVCSStatusNoProjects,
userConfig.QuietPolicyChecks,
)

We'd also need some tests with this flag and without this flag.

Honestly, it might be better to revert the pr for now and cut a v0.22.1 release.

@nitrocode nitrocode added the bug Something isn't working label Jan 3, 2023
@nitrocode nitrocode changed the title Discard approvals by default in v0.22.0 Discard approvals by default in v0.22.0 due to missing flag logic Jan 3, 2023
@nitrocode nitrocode pinned this issue Jan 3, 2023
@nitrocode nitrocode changed the title Discard approvals by default in v0.22.0 due to missing flag logic Discard approvals by default in v0.22.0 due to missing logic Jan 3, 2023
@nitrocode
Copy link
Member Author

Looking for people to test the new dev image out before cutting a 0.22.1 release.

cc @mcantinqc @secustor

@nitrocode nitrocode unpinned this issue Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant