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

ptl-tool: make branch name configurable #18499

Merged
merged 1 commit into from Oct 31, 2017

Conversation

Projects
None yet
2 participants
@batrick
Copy link
Member

commented Oct 24, 2017

Also, include hours/minutes in branch so multiple branches in a day have
different names.

Signed-off-by: Patrick Donnelly pdonnell@redhat.com

@joscollin

This comment has been minimized.

Copy link
Member

commented Oct 24, 2017

Also, include hours/minutes in branch so multiple branches in a day have
different names.

I thought of creating this as a PR after seeing it in build-integration-branch and then I forgot somehow :-)

with open(expanduser("~/.github.key")) as f:
PASSWORD = f.read().strip()
BRANCH_PREFIX = "wip-%s-testing-" % USER
TESTING_BRANCH_NAME = BRANCH_PREFIX + datetime.datetime.now().strftime("%Y%m%d")
TEST_BRANCH = os.getenv("PTL_TOOL_TEST_BRANCH", "wip-{user}-testing-%Y%m%d%H%M")

This comment has been minimized.

Copy link
@joscollin

joscollin Oct 24, 2017

Member

You could add seconds too, just to make it perfect. It is possible for the user to execute the command 2 times within the same minute.

Want to provide - or _ in between the time ?

This comment has been minimized.

Copy link
@joscollin

joscollin Oct 24, 2017

Member

Is --merge-branch-name option still valid ? If not please update the readme ?

$ src/script/ptl-tool.py --base master --merge-branch-name wip-jcollin-testing --pr-label wip-ptltool1-testing
The new change overrides --merge-branch-name now

This comment has been minimized.

Copy link
@batrick

batrick Oct 24, 2017

Author Member

The new change overrides --merge-branch-name now

I don't think so?

$ src/script/ptl-tool.py --branch HEAD --merge-branch-name master 18192
Will merge PRs: [18192]
Detaching HEAD onto base: master
Merging PR #18192
Leaving HEAD detached; no branch anchors your commits
$ git log -1
commit 6fa6ccff4f195a5744b9a0419f9e98982a8c0d55 (HEAD)
Merge: bc139f02d2 d0732fc96f
Author: Patrick Donnelly <pdonnell@redhat.com>
Date:   Mon Oct 23 22:30:35 2017 -0700

    Merge PR #18192 into master
    
    * refs/pull/18192/head:
            qa/cephfs: test ec data pool
            qa/suites/fs/basic_functional/clusters: more osds
    
    Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

and without branch

$ src/script/ptl-tool.py --merge-branch-name master 18192
Will merge PRs: [18192]
Detaching HEAD onto base: master
Merging PR #18192
Checked out new branch wip-pdonnell-testing-20171023
Created tag testing/wip-pdonnell-testing-20171023
$ git log -1
commit 4c79bc7daf72a1f9153e78112ae8223326e25fd1 (HEAD -> wip-pdonnell-testing-20171023, tag: testing/wip-pdonnell-testing-20171023)
Merge: bc139f02d2 d0732fc96f
Author: Patrick Donnelly <pdonnell@redhat.com>
Date:   Mon Oct 23 22:32:03 2017 -0700

    Merge PR #18192 into master
    
    * refs/pull/18192/head:
            qa/cephfs: test ec data pool
            qa/suites/fs/basic_functional/clusters: more osds
    
    Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

This comment has been minimized.

Copy link
@batrick

batrick Oct 24, 2017

Author Member

Whoops second example should be:

 src/script/ptl-tool.py --merge-branch-name master 18192
Will merge PRs: [18192]
Detaching HEAD onto base: master
Merging PR #18192
Checked out new branch wip-pdonnell-testing-201710240533
Created tag testing/wip-pdonnell-testing-201710240533
$ git log -1
commit 62f2ddb51eaf6f6b553d978a8ad764fb2e8f8bb4 (HEAD -> wip-pdonnell-testing-201710240533, tag: testing/wip-pdonnell-testing-201710240533)
Merge: bc139f02d2 d0732fc96f
Author: Patrick Donnelly <pdonnell@redhat.com>
Date:   Mon Oct 23 22:33:11 2017 -0700

    Merge PR #18192 into master
    
    * refs/pull/18192/head:
            qa/cephfs: test ec data pool
            qa/suites/fs/basic_functional/clusters: more osds
    
    Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

This comment has been minimized.

Copy link
@batrick

batrick Oct 24, 2017

Author Member

Updated the PR to add seconds.

This comment has been minimized.

Copy link
@joscollin

joscollin Oct 24, 2017

Member

@batrick Could you please try this ? Is this desired behavior ?

$ export PTL_TOOL_USER=joscollin
$ src/script/ptl-tool.py --merge-branch-name wip-jcollin-testing 18192
Will merge PRs: [18192]
Detaching HEAD onto base: master
Merging PR #18192
Checked out new branch wip-joscollin-testing-20171024.065225
Created tag testing/wip-joscollin-testing-20171024.065225

This comment has been minimized.

Copy link
@batrick

batrick Oct 24, 2017

Author Member

Yes, that looks right. I added a decimal point because it's difficult to read the date/time otherwise.

@batrick batrick force-pushed the batrick:ptl-tool-testing-time branch from dcfffaf to 91da4e4 Oct 24, 2017

@batrick batrick closed this Oct 25, 2017

@batrick batrick deleted the batrick:ptl-tool-testing-time branch Oct 25, 2017

@batrick batrick restored the batrick:ptl-tool-testing-time branch Oct 25, 2017

@batrick batrick reopened this Oct 25, 2017

@batrick

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2017

@joscollin looks good to merge?

@joscollin

This comment has been minimized.

Copy link
Member

commented Oct 26, 2017

looks good to merge?

@batrick Let me check once more.

@@ -293,16 +295,14 @@ def main():
else:
argv = sys.argv[1:]
parser.add_argument('--branch', dest='branch', action='store', default=default_branch, help='branch to create ("HEAD" leaves HEAD detached; i.e. no branch is made)')
parser.add_argument('--merge-branch-name', dest='merge_branch_name', action='store', help='name of the branch for merge messages')
parser.add_argument('--merge-branch-name', dest='merge_branch_name', action='store', default=False, help='name of the branch for merge messages')

This comment has been minimized.

Copy link
@joscollin

joscollin Oct 26, 2017

Member

@batrick I don't understand what is the relevance of --merge-branch-name now. I think this option can be removed, as branch name is configurable and automated by appending time to it. Am I missing something here ? Please clarify. Thanks.

This comment has been minimized.

Copy link
@batrick

batrick Oct 26, 2017

Author Member

It is useful in conjunction with --branch HEAD. I use it frequently when merging PRs to master:

ptl-tool.py --branch HEAD --merge-branch-name master 12345

This comment has been minimized.

Copy link
@batrick

batrick Oct 26, 2017

Author Member

(A peculiarity of my setup is that I don' thave a local master branch because I find it useless.)

@joscollin joscollin removed the needs-review label Oct 27, 2017

@joscollin

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

@batrick The testing branch name is created using getuser() with no time appended, when I execute the script for the second time ?

jcollin@smithi042:~/test-ptl-18499$ echo $PTL_TOOL_USER
jos
jcollin@smithi042:~/test-ptl-18499$ src/script/ptl-tool.py --base master --pr-label wip-ptltool1-testing
Adding labeled PR #18284 to PR list
Will merge PRs: [18284]
Detaching HEAD onto base: master
Merging PR #18284
Checked out new branch wip-jos-testing-20171027.043558
Created tag testing/wip-jos-testing-20171027.043558
jcollin@smithi042:~/test-ptl-18499$ git branch
  18499
  master
* wip-jos-testing-20171027.043558
jcollin@smithi042:~/test-ptl-18499$ src/script/ptl-tool.py --base master --pr-label wip-ptltool1-testing
Adding labeled PR #18284 to PR list
Will merge PRs: [18284]
Detaching HEAD onto base: master
Merging PR #18284
Checked out new branch wip-jcollin-testing-20171027
Created tag testing/wip-jcollin-testing-20171027_03
jcollin@smithi042:~/test-ptl-18499$ git branch
  18499
  master
* wip-jcollin-testing-20171027
  wip-jos-testing-20171027.043558
@batrick

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2017

@joscollin the issue is that you were using the master version of ptl-tool.py when you executed it the second time.

@joscollin

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

@batrick Yes, that seems to be a mistake on my side. I'm not able to reproduce it now. :-)

jcollin@smithi042:/test-ptl-18499_10-31-2017$ export PTL_TOOL_USER=
jcollin@smithi042:
/test-ptl-18499_10-31-2017$ src/script/ptl-tool.py --base master --pr-label wip-ptltool1-testing
Adding labeled PR #18633 to PR list
Will merge PRs: [18633]
Detaching HEAD onto base: master
Merging PR #18633
[ 4343a21 ] BZ cited: Related-to: https://bugzilla.redhat.com/show_bug.cgi?id=684400
[ https://github.com//pull/18633#pullrequestreview-73043634 ] Ceph tracker cited: http://tracker.ceph.com/issues/21000
[ https://github.com//pull/18633#pullrequestreview-73049713 ] BZ cited: Related-to: https://bugzilla.redhat.com/show_bug.cgi?id=684400
[ https://github.com//pull/18633#pullrequestreview-73049713 ] Ceph tracker cited: Fixes: http://tracker.ceph.com/issues/21210
Need name for contributor "aniyanrajan"; Reviewed-by: Aniyan Rajan aniyan.rajan@gmail.com
adding new contributors to githubmap on top of PR #18633
Checked out new branch wip--testing-20171031.061659
Created tag testing/wip--testing-20171031.061659

How about checking PTL_TOOL_USER set to an empty value ? If it is empty, then let it take the username.

@batrick

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2017

How about checking PTL_TOOL_USER set to an empty value ? If it is empty, then let it take the username.

The right thing to do is to unset PTL_TOOL_USER in the calling shell.

@joscollin

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

@batrick Please resolve the conflicts

ptl-tool: make branch name configurable
Also, include hours/minutes in branch so multiple branches in a day have
different names.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

@batrick batrick force-pushed the batrick:ptl-tool-testing-time branch from 91da4e4 to 9902f5c Oct 31, 2017

@batrick

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2017

done

@joscollin joscollin merged commit 42b2cc1 into ceph:master Oct 31, 2017

4 of 5 checks passed

make check (arm64) running make check
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

@batrick batrick deleted the batrick:ptl-tool-testing-time branch Dec 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.