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

Required reviewer not being removed if automation not run #375

Open
matthewparavati opened this issue Dec 6, 2023 · 10 comments
Open

Required reviewer not being removed if automation not run #375

matthewparavati opened this issue Dec 6, 2023 · 10 comments
Labels

Comments

@matthewparavati
Copy link

Describe the bug

We have an automation (do_not_merge) to block merging if the "Do Not Merge" label is added. When the label is added, a required reviewer is set, and the PR is blocked from merging. When removing the label, the required reviewer is not removed and the PR is still blocked from merging.

To Reproduce

Steps to reproduce the behavior:

  1. The .cm automation file
gitstream.cm
# -*- mode: yaml -*-

manifest:
  version: 1.0

automations:
  estimated_time_to_review:
    if:
      - true
    run:
      - action: add-label@v1
        args:
          label: "~{{ calc.etr }} min review"
          color: {{ 'E94637' if (calc.etr >= 20) else ('FBBD10' if (calc.etr >= 5) else '36A853') }}
  needs_one_review:
    if:
      - {{ not has.sensitive_files }}
      - {{ is.quick_review }}
      - {{ approvals.zero }}
    run:
      - action: add-label@v1
        args: 
          label: ⏳ Waiting for 1 reviewer
          color: '#73067C'
      - action: add-reviewers@v1
        args:
          reviewers: [Popmenu/mobile-app-devs]
          unless_reviewers_set: true
  needs_two_reviews:
    if:
      - {{ is.long_review or has.sensitive_files }}
      - {{ approvals.ltTwo }}
    run:
      - action: add-label@v1
        args: 
          label: {{ '⏳ Waiting for 2 reviewers' if (approvals.zero) else '⏳ Waiting for 1 reviewer' }}
          color: '#73067C'
      - action: add-reviewers@v1
        args:
          reviewers: [Popmenu/mobile-app-devs]
          unless_reviewers_set: true
  deleted:
    if:
      - {{ has.deleted_files }}
    run: 
      - action: add-label@v1
        args:
          label: 🗑️ Deleted files
          color: '#DF9C04'
  missing_sc_ticket:
    if:
      - {{ not sc_ticket.comment }}
    run: 
      - action: add-label@v1
        args:
          label: 🎫 Missing SC Ticket
          color: '#FBCA02'
  unresolved_threads:
    if:
      - {{ pr.status == 'open' }}
      - {{ pr.unresolved_threads > 0 }}
    run:
      - action: add-label@v1
        args:
          label: 🚨 Unresolved thread(s)
          color: '#EC560D'
  sensitive_file_change_review:
    if: 
      - {{ has.sensitive_files }}
    run:
      - action: set-required-approvals@v1
        args:
          approvals: 2
  two_reviews:
    if: 
      - {{ is.long_review or has.sensitive_files }}
    run:
      - action: set-required-approvals@v1
        args:
          approvals: 2
  do_not_merge:
    if:
      - {{ has.do_not_merge_label }}
    run:
      - action: require-reviewers@v1
        args:
          reviewers: [john-dugan]
          also_assign: false
  ready_to_merge_one_approval_pr:
    if:
      - {{ not has.sensitive_files }}
      - {{ is.quick_review }}
      - {{ not has.do_not_merge_label }}
      - {{ approvals.gtZero }}
    run:
      - action: add-label@v1
        args:
          label: ✌️ Ready to merge
          color: '#219944'
  ready_to_merge_two_approval_pr:
    if:
      - {{ is.long_review or has.sensitive_files }}
      - {{ not has.do_not_merge_label }}
      - {{ approvals.gtOne }}
    run:
      - action: add-label@v1
        args:
          label: ✌️ Ready to merge
          color: '#219944'
  
calc:
  etr: {{ branch | estimatedReviewTime }}
has:
  deleted_files: {{ source.diff.files | map(attr='new_file') | match(term='/dev/null') | some }}
  sensitive_files: {{ files | match(list=sensitive) | some }}
  do_not_merge_label: {{ pr.labels | match(term='Do not merge') | some }}
is:
  safe_change: {{ (source.diff.files | isFormattingChange) or (files | allDocs) or (files | allTests) or (files | allImages) }}
  quick_review: {{ files | length <= 7 and calc.etr <= 5 }}
  long_review: {{ files | length > 7 or calc.etr > 5 }}
approvals:
  zero: {{ pr.approvals | length == 0 }}
  gtZero: {{ pr.approvals | length > 0 }}
  gtOne: {{ pr.approvals | length > 1 }}
  ltTwo: {{ pr.approvals | length < 2 }}
sc_ticket:
  comment: {{ pr.comments | match(attr='content', term='Shortcut Story') | some }}
sensitive:
  - App.tsx
  - AppRoot.tsx
config:
  ignore_files:
    - 'yarn.lock'
    - 'ios/*.lock'
    - 'android/*.lock'
    - 'src/graphql/schema.json'
    - 'src/__generated__/types.ts'
    - 'src/i18n/messages/*.json'
    - '__tests__/**/*.snap'
  1. PR Url

  2. Describe your PR relevant content

  3. Add relevant commit SHA

Expected behavior

When the "do not merge" label is removed from a PR, then the required reviewer is removed, since the require-reviewers action did not run and the PR is no longer blocked from merging.

Screenshots

Expected results when adding "do not merge"
Screenshot 2023-12-06 at 07 56 47

Unexpected result after removing "do not merge" where PR is still blocked from merging

Screenshot 2023-12-06 at 07 59 28

Additional context

Previous issue discussing this implementation

This was working fine for a bit but more recently, after removing the label, the PR is still blocked because of the required reviewer that was previously added. I've tried to update the automation with the beta also_assign argument but that didn't resolve this issue

@matthewparavati matthewparavati added the bug Something isn't working label Dec 6, 2023
@BenLloydPearson
Copy link
Collaborator

@matthewparavati Thanks for reporting this issue and providng a link to a related PR. Would you mind also sharing the CM file you're using for this automation? This will help us debug the problem and get back to you.

Thanks!

@matthewparavati
Copy link
Author

@BenLloydPearson I've added it as a part of the collapsible text section gitstream.cm in my initial report. Is that fine?

@BenLloydPearson
Copy link
Collaborator

Perfect! Thank you! We'll dig into this and get back to you.

@vim-zz
Copy link
Collaborator

vim-zz commented Dec 12, 2023

@matthewparavati currently the action require-reviewers@v1 is not managed, meaning it is not being re-evaluated unless there is a new commit

@matthewparavati
Copy link
Author

@vim-zz thanks for the response. The require-reviewers@v1 action was previously managed, correct? and this behavior was changed recently?

Is there a different action I should/could use instead to block merging if a label is added and then allow merging when the label is removed?

@vim-zz
Copy link
Collaborator

vim-zz commented Dec 12, 2023

The request-changes is managed and can be used to block PRs: https://docs.gitstream.cm/automation-actions/#request-changes

@matthewparavati
Copy link
Author

matthewparavati commented Dec 13, 2023

@vim-zz Thanks, I tried updating to using the request-changes action. It correctly blocked the PR once we added the label but when we removed the label, "request-changes" still shows as a failing check and blocks the PR for some devs. I assume if the gitstream review is resolved and the action is not run, then PR should no longer be blocked from merging, correct?

Screenshot 2023-12-13 at 16 48 25

PR with failing check

gitstream.cm
# -*- mode: yaml -*-

manifest:
  version: 1.0

automations:
  estimated_time_to_review:
    if:
      - true
    run:
      - action: add-label@v1
        args:
          label: "~{{ calc.etr }} min review"
          color: {{ 'E94637' if (calc.etr >= 20) else ('FBBD10' if (calc.etr >= 5) else '36A853') }}
  needs_one_review:
    if:
      - {{ not has.sensitive_files }}
      - {{ is.quick_review }}
      - {{ approvals.zero }}
    run:
      - action: add-label@v1
        args: 
          label: ⏳ Waiting for 1 reviewer
          color: '#73067C'
      - action: add-reviewers@v1
        args:
          reviewers: [Popmenu/mobile-app-devs]
          unless_reviewers_set: true
  needs_two_reviews:
    if:
      - {{ is.long_review or has.sensitive_files }}
      - {{ approvals.ltTwo }}
    run:
      - action: add-label@v1
        args: 
          label: {{ '⏳ Waiting for 2 reviewers' if (approvals.zero) else '⏳ Waiting for 1 reviewer' }}
          color: '#73067C'
      - action: add-reviewers@v1
        args:
          reviewers: [Popmenu/mobile-app-devs]
          unless_reviewers_set: true
  deleted:
    if:
      - {{ has.deleted_files }}
    run: 
      - action: add-label@v1
        args:
          label: 🗑️ Deleted files
          color: '#DF9C04'
  missing_sc_ticket:
    if:
      - {{ not sc_ticket.comment }}
    run: 
      - action: add-label@v1
        args:
          label: 🎫 Missing SC Ticket
          color: '#FBCA02'
  unresolved_threads:
    if:
      - {{ pr.status == 'open' }}
      - {{ pr.unresolved_threads > 0 }}
    run:
      - action: add-label@v1
        args:
          label: 🚨 Unresolved thread(s)
          color: '#EC560D'
  sensitive_file_change_review:
    if: 
      - {{ has.sensitive_files }}
    run:
      - action: set-required-approvals@v1
        args:
          approvals: 2
  two_reviews:
    if: 
      - {{ is.long_review or has.sensitive_files }}
    run:
      - action: set-required-approvals@v1
        args:
          approvals: 2
  do_not_merge:
    if:
      - {{ has.do_not_merge_label }}
    run:
      - action: request-changes@v1
        args:
          comment: |
            Merging is blocked while "Do Not Merge" label is added
  ready_to_merge_one_approval_pr:
    if:
      - {{ not has.sensitive_files }}
      - {{ is.quick_review }}
      - {{ not has.do_not_merge_label }}
      - {{ approvals.gtZero }}
    run:
      - action: add-label@v1
        args:
          label: ✌️ Ready to merge
          color: '#219944'
  ready_to_merge_two_approval_pr:
    if:
      - {{ is.long_review or has.sensitive_files }}
      - {{ not has.do_not_merge_label }}
      - {{ approvals.gtOne }}
    run:
      - action: add-label@v1
        args:
          label: ✌️ Ready to merge
          color: '#219944'
  
calc:
  etr: {{ branch | estimatedReviewTime }}
has:
  deleted_files: {{ source.diff.files | map(attr='new_file') | match(term='/dev/null') | some }}
  sensitive_files: {{ files | match(list=sensitive) | some }}
  do_not_merge_label: {{ pr.labels | match(term='Do not merge') | some }}
is:
  safe_change: {{ (source.diff.files | isFormattingChange) or (files | allDocs) or (files | allTests) or (files | allImages) }}
  quick_review: {{ files | length <= 7 and calc.etr <= 5 }}
  long_review: {{ files | length > 7 or calc.etr > 5 }}
approvals:
  zero: {{ pr.approvals | length == 0 }}
  gtZero: {{ pr.approvals | length > 0 }}
  gtOne: {{ pr.approvals | length > 1 }}
  ltTwo: {{ pr.approvals | length < 2 }}
sc_ticket:
  comment: {{ pr.comments | match(attr='content', term='Shortcut Story') | some }}
sensitive:
  - App.tsx
  - AppRoot.tsx
config:
  ignore_files:
    - 'yarn.lock'
    - 'ios/*.lock'
    - 'android/*.lock'
    - 'src/graphql/schema.json'
    - 'src/__generated__/types.ts'
    - 'src/i18n/messages/*.json'
    - '__tests__/**/*.snap'

@matthewparavati
Copy link
Author

@vim-zz @BenLloydPearson thoughts on the above? Is it easier/better to create a new issue?

@vim-zz
Copy link
Collaborator

vim-zz commented Feb 18, 2024

@matthewparavati this will be addressed as a roadmap item

@vim-zz vim-zz added roadmap and removed bug Something isn't working labels Feb 18, 2024
@codfish
Copy link

codfish commented May 1, 2024

@vim-zz any insight in what the ETA is for this feature/fix? For context, here's a snippet of our use case which seems pretty similar to the authors. We just have to comment it out for now cause it doesn't resolve itself so it blocks devs without manual intervention so not a great experience.

  enforce_labels:
    if:
      - {{ pr.draft == false }}
      - {{ pr.approvals | length >= 2 }}
      - {{ pr.labels | match(regex=r/^(needs .*|do not merge)$/i) }}
    run:
      - action: request-changes@v1
        args:
          comment: |
            Could you please flip ....

Also curious what the end solution will look like? Will you just programmatically dismiss the review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants