advise PR labels in --review-pr and add support for --add-pr-labels

[migueldiascosta]

No description provided.

[boegel]

Still missing a test?

[migueldiascosta]

@akesandgren thanks, too much copy-pasting...

I'd like to know if you (or any other maintainer) might think it's not adequate for --review-pr to post labels - for instance, I can imagine arguing that it's expected to be a read-only operation...

[boegel]

@migueldiascosta The additional work to add support for --add-pr-labels seems rather minimal at this point, why not add it to this PR?

[boegel]

@migueldiascosta Just in case you missed my suggestion, adding support for --add-pr-labels should be fairly trivial now, no?

[migueldiascosta]

some of the tests are fragile in that they will fail if the labels of the (closed) PRs used ever change...

[boegel]

@migueldiascosta Should we open PRs in https://github.com/easybuilders/testrepository to prevent us some breaking the tests?
We could mention in the PR title to leave the labels and add a reference to the test that relies on it?

[migueldiascosta]

@boegel add_pr_labels uses fetch_easyconfigs_from_pr which expects an easybuild-easyconfigs repo

We could use any fork of easybuild-easyconfigs, e.g. boegelbot's, ebtutorial's or a new test account

[boegel]

@migueldiascosta Needs a sync with develop to resolve the merge conflict...

[boegel]

@migueldiascosta We can open a PR in @boegelbot's fork for this, sure. Can you look into this?

[boegel]

@migueldiascosta This now also needs a sync with develop to fix the merge conflicts...

[boegelbot]

@migueldiascosta: Tests failed in GitHub Actions, see https://github.com/easybuilders/easybuild-framework/actions/runs/392881152
Output from first failing test suite run:

FAIL: test_easystack_basic (test.framework.options.CommandLineOptionsTest)
Test for --easystack <easystack.yaml> -> success case
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/framework/options.py", line 5653, in test_easystack_basic
    self.assertTrue(regex.match(stdout) is not None)
AssertionError: False is not true

----------------------------------------------------------------------
Ran 743 tests in 916.296s

FAILED (failures=1)
ERROR: Not all tests were successful.

bleep, bloop, I'm just a bot (boegelbot v20200716.01)
Please talk to my owner @boegel if you notice you me acting stupid),
or submit a pull request to https://github.com/boegel/boegelbot fix the problem.

[migueldiascosta]

easystack_basic test is failing with unbound method error() must be called with EasyStackParser instance as first argument (got str instance instead), not sure how this is related to this PR...

[boegel]

@migueldiascosta I ran into that while working on #3479, but that should be fixed in develop...

I don't see that error popping up in the CI logs though, are you only seeing that when running that test locally?

[migueldiascosta]

I don't see that error popping up in the CI logs though, are you only seeing that when running that test locally?

it is popping up in the CI tests, e.g. https://github.com/easybuilders/easybuild-framework/pull/3177/checks?check_run_id=1477507461

[migueldiascosta]

I don't see that error popping up in the CI logs though, are you only seeing that when running that test locally?

sorry, yes, the actual error message I printed locally, it should show up in the CI now that your #3511 is merged (both to develop and to this branch)

[migueldiascosta]

except now it doesn't fail in the CI...

fwiw, that test still fails for me locally (with python 2.7.5; it passes with python 3.6.8)

[boegel]

@migueldiascosta Can you open an issue with more info on how the test fails exactly with current develop?