bots: Distinguish context only when repo is different to PR target #12371

marusak · 2019-07-21T15:43:35Z

I have question about this and I am opening this PR to discuss it.

The test project (for now) in the tests-invoke is used just for building up name for publishing. I am not sure what this all affects, I've changed it in this PR to always use project name. If this has unwanted effect then I am ready to change it to if self.test_project != "cockpit-project/cockpit.

@Gundersanne @martinpitt

martinpitt · 2019-07-22T06:13:02Z

This looks fine. It's another nice and required step towards making bots/ independent, and gets rid of more special cases. I just wonder why this is no-test but has all the cockpit tests, which apparently did not get run? How did you trigger these?

I'll re-trigger one manually.

marusak · 2019-07-22T06:39:46Z

I just wonder why this is no-test but has all the cockpit tests

Because I opened it and forgot to add [no-test] label. I've noticed it immediately hoping that I can make it before it is processed (now thinking that we are sending these data in json and it does not read it from github so there was no chance at all)

martinpitt · 2019-07-22T06:42:08Z

Actually no: It immediately crashes:

sh-5.0$ PRIORITY=0005 bots/make-checkout --verbose --base=master --repo=cockpit-project/cockpit pull/12371/head 545f45d81840755309e72e2382131366d9211075 && GITHUB_BASE=cockpit-project/cockpit TEST_NAME=pull-12371-20190722-063838 TEST_REVISION=545f45d81840755309e72e2382131366d9211075 bots/tests-invoke --pull-number=12371  --rebase=master avocado/fedora pull/12371/head
+ git fetch origin pull/12371/head
From https://github.com/cockpit-project/cockpit
 * branch                refs/pull/12371/head -> FETCH_HEAD
+ git checkout --detach 545f45d81840755309e72e2382131366d9211075
Previous HEAD position was 6229f51ab images: Update continuous-atomic image
HEAD is now at 545f45d81 bots: Always pass TEST_PROJECT to tests-invoke
Traceback (most recent call last):
  File "bots/tests-invoke", line 396, in <module>
    sys.exit(main())
  File "bots/tests-invoke", line 64, in main
    ret = task.run(opts)
  File "bots/tests-invoke", line 300, in run
    self.start_publishing(opts.publish)
  File "bots/tests-invoke", line 116, in start_publishing
    id_context = "-".join([self.test_project, self.context])
TypeError: sequence item 0: expected str instance, NoneType found

You should be able to reproduce this locally, and then make it deal with this situation.

martinpitt · 2019-07-22T06:42:54Z

This happens for this PR because tests-scan runs from master, while tests-invoke runs from this branch.

marusak · 2019-07-22T07:14:38Z

This happens for this PR because tests-scan runs from master, while tests-invoke runs from this branch.

Yeah, that make sense. Should I really check if it is set? Normally this won't happen

martinpitt · 2019-07-22T07:36:16Z

@marusak: Alternatively, land the tests-scan change first in a separate PR.

marusak · 2019-07-22T07:44:46Z

Blocked on #12380

martinpitt · 2019-07-22T07:48:26Z

Landed the other one, please rebase.

marusak · 2019-07-22T07:52:59Z

Rebased

martinpitt

Thanks. This is now purely clean-up, and should not block anything any more, right?

martinpitt · 2019-07-22T07:54:19Z

Triggering one test, to make sure this works now.

marusak · 2019-07-22T07:55:12Z

This is now purely clean-up, and should not block anything any more, right?

Indeed.

Seems to cause a regression

martinpitt · 2019-07-22T07:56:12Z

Hold on -- it now appends @cockpit-project/cockpit to the test. We shouldn't do this if the project is equal to the PR target.

marusak · 2019-07-22T08:01:04Z

It is due to #12380. Either that needs to pass this variable only when project is not equal to PR target and this PR is then obsoleted or this PR needs to do this check. (I am in favor of the second one)

martinpitt · 2019-07-22T08:03:25Z

or this PR needs to do this check. (I am in favor of the second one)

Yes, let's do that one. Always passing $TEST_PROJECT sounds cleaner IMHO.

marusak · 2019-07-22T08:24:37Z

I've triggered avocado/fedora and cockpit/rhel-atomic@cockpit-project/cockpit-ostree. Seems that both got picked up correctly.

martinpitt · 2019-07-22T08:25:56Z

bots/tests-invoke

@@ -114,7 +114,7 @@ class PullTask(object):

        # build a unique file name for this test run
        id_context = self.context
-        if self.test_project:  # disambiguate when running several external projects
+        if self.test_project != api.repo :  # disambiguate when running several external projects


This is just for the log name, right? I think this special case could go, it doesn't matter if the log name contains the cockpit project.

martinpitt · 2019-07-22T08:27:02Z

bots/tests-invoke

@@ -126,7 +126,7 @@ class PullTask(object):

        # build a globally unique test context for GitHub statuses
        github_context = self.context
-        if self.test_project:  # disambiguate tests for external projects
+        if self.test_project != api.repo :  # disambiguate tests for external projects


The code change is right, but the comment is now misleading. How about "disambiguate test name for foreign project tests"?

martinpitt

Thanks!

marusak · 2019-07-22T08:53:01Z

I am just waiting if avocado/fedora gets picked up. Lets please wait for that one.

marusak added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jul 21, 2019

cockpituous added the test-external label Jul 21, 2019

marusak mentioned this pull request Jul 21, 2019

Normalize tests invocation #12367

Merged

2 tasks

martinpitt added the needswork label Jul 22, 2019

marusak mentioned this pull request Jul 22, 2019

bots: Always pass TEST_PROJECT to tests-invoke no.2 #12380

Merged

marusak added the blocked Don't land until something else happens first (see task list) label Jul 22, 2019

martinpitt removed blocked Don't land until something else happens first (see task list) needswork labels Jul 22, 2019

marusak force-pushed the testprojectforall branch from 545f45d to 8dde5fe Compare July 22, 2019 07:52

martinpitt previously approved these changes Jul 22, 2019

View reviewed changes

marusak force-pushed the testprojectforall branch from 8dde5fe to 567fbc8 Compare July 22, 2019 08:20

martinpitt requested changes Jul 22, 2019

View reviewed changes

bots: Distinguish context only when repo is different to PR target

63c36dd

marusak force-pushed the testprojectforall branch from 567fbc8 to 63c36dd Compare July 22, 2019 08:36

martinpitt approved these changes Jul 22, 2019

View reviewed changes

martinpitt changed the title ~~bots: Always pass TEST_PROJECT to tests-invoke~~ bots: Distinguish context only when repo is different to PR target Jul 22, 2019

martinpitt merged commit c89aa36 into cockpit-project:master Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bots: Distinguish context only when repo is different to PR target #12371

bots: Distinguish context only when repo is different to PR target #12371

marusak commented Jul 21, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt left a comment

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt Jul 22, 2019

martinpitt Jul 22, 2019

martinpitt left a comment

marusak commented Jul 22, 2019 •

edited

Loading

bots: Distinguish context only when repo is different to PR target #12371

bots: Distinguish context only when repo is different to PR target #12371

Conversation

marusak commented Jul 21, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak commented Jul 22, 2019

martinpitt Jul 22, 2019

Choose a reason for hiding this comment

martinpitt Jul 22, 2019

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

marusak commented Jul 22, 2019 • edited Loading

marusak commented Jul 22, 2019 •

edited

Loading