Skip to content

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

Merged
boegel merged 32 commits intoeasybuilders:developfrom
migueldiascosta:review_pr_labels
Feb 19, 2021
Merged

advise PR labels in --review-pr and add support for --add-pr-labels#3177
boegel merged 32 commits intoeasybuilders:developfrom
migueldiascosta:review_pr_labels

Conversation

@migueldiascosta
Copy link
Copy Markdown
Member

No description provided.

@boegel boegel added this to the next release (4.1.2?) milestone Feb 17, 2020
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing a test?

Comment thread easybuild/framework/easyconfig/tools.py Outdated
Comment thread easybuild/tools/github.py Outdated
Comment thread easybuild/tools/github.py Outdated
@migueldiascosta
Copy link
Copy Markdown
Member Author

@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...

Comment thread easybuild/framework/easyconfig/tools.py Outdated
Comment thread easybuild/framework/easyconfig/tools.py Outdated
@easybuilders easybuilders deleted a comment from boegelbot Apr 8, 2020
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread easybuild/framework/easyconfig/tools.py Outdated
Comment thread easybuild/framework/easyconfig/tools.py Outdated
Comment thread easybuild/tools/github.py Outdated
Comment thread easybuild/tools/github.py Outdated
Comment thread test/framework/github.py Outdated
Comment thread test/framework/github.py Outdated
Comment thread test/framework/github.py Outdated
Comment thread test/framework/github.py Outdated
Comment thread test/framework/options.py
@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 11, 2020

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

@migueldiascosta
Copy link
Copy Markdown
Member Author

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

@migueldiascosta migueldiascosta changed the title advise PR labels in --review-pr and add support for --add-pr-labels (WIP) advise PR labels in --review-pr and add support for --add-pr-labels (REVIEW) Sep 16, 2020
@boegel
Copy link
Copy Markdown
Member

boegel commented Sep 30, 2020

@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
Copy link
Copy Markdown
Member Author

@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
Copy link
Copy Markdown
Member

boegel commented Oct 14, 2020

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

@boegel boegel modified the milestones: 4.3.1 (next release), 4.3.2 Oct 25, 2020
@boegel
Copy link
Copy Markdown
Member

boegel commented Nov 30, 2020

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

@boegel boegel modified the milestones: 4.3.2 (next release), 4.4.0 Nov 30, 2020
@boegel
Copy link
Copy Markdown
Member

boegel commented Nov 30, 2020

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

@boegelbot
Copy link
Copy Markdown

@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
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Member

boegel commented Dec 1, 2020

@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
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Member Author

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)

@boegel boegel changed the title advise PR labels in --review-pr and add support for --add-pr-labels (REVIEW) advise PR labels in --review-pr and add support for --add-pr-labels Dec 1, 2020
@migueldiascosta
Copy link
Copy Markdown
Member Author

migueldiascosta commented Dec 1, 2020

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
Copy link
Copy Markdown
Member

boegel commented Dec 1, 2020

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

@boegel boegel merged commit 84e0585 into easybuilders:develop Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants