openSUSE / gitarro

run all your test against a GitHub Pull request
https://opensuse.github.io/gitarro
MIT License
15 stars 20 forks source link

Gitarro --check flag #166

Closed srbarrios closed 5 years ago

srbarrios commented 5 years ago

Hi,

After run gitarro.ruby2.1 -r ${repository} -t placeholder --check --changed_since 3600 if there is something to retrigger, so I have a comment or checked_checkbox with a test to rerun, it will change the status in github addind a "pending" status for that test.

  # public for retrigger the test
  def retrigger_check(pr)
    return unless retrigger_needed?(pr)

    create_status(pr, 'pending')
    print_test_required
    gbexec.export_pr_data(pr)
    exit 0 if @check
    launch_test_and_setup_status(pr)
  end

Is that behavior correct?

If that's correct I can't use the --check flag to avod using a GitHub webhook. I'm checking without filters, as I need to check for all the possible tests available. But then it will set to pending a test, even if that test will have a filter on my "real" gitarro execution.

I suggest to create a flag --no_pending to don't add the pending status or change the code and remove that create_status from that point:

  # public for retrigger the test
  def retrigger_check(pr)
    return unless retrigger_needed?(pr)

    print_test_required
    gbexec.export_pr_data(pr)
    exit 0 if @check
    launch_test_and_setup_status(pr)
  end

As in the last step launch_test_and_setup_status(pr) we are already calling setting the status, so it will be set two times to pending when there is no --check flag.

  def launch_test_and_setup_status(pr)
    # pending
    create_status(pr, 'pending')
    # do tests
    test_status = gbexec.pr_test(pr)
    # set status
    create_status(pr, test_status)
    # return status for other functions
    test_status == 'success' ? exit(0) : exit(1)
  end
MalloZup commented 5 years ago

If check set a pending flag we need to fix this without adding an extra cmdline. Adding flags like this will open lot of issues and is not minimalistic, in this context I don't see also the need for that cmdline

srbarrios commented 5 years ago

The issue now is that if we fix it, we will change the current behavior and maybe someone was using it.

MalloZup commented 5 years ago

on the main design, i don't think check should add pending so we threat this as bug

srbarrios commented 5 years ago

on the main design, i don't think check should add pending so we threat this as bug

I have the same opinion, yes. I was just worried about that change of behavior, but if we agreed on it, perfect, easier to fix on my side.

MalloZup commented 5 years ago

ok good that you open an issue for dicussing it. :+1:

MalloZup commented 5 years ago

This GitHub PR is unactive since more then 30 days. Is this GitHub PR still needed? Please close or update it accordingly. This reminder is autogenerated by https://github.com/MalloZup/blacktango

srbarrios commented 5 years ago

That task is done.