Closed
Bug 1286897
Opened 8 years ago
Closed 8 years ago
Fix wrong python naming convention in runnable_jobs.py
Categories
(Tree Management :: Treeherder, defect)
Tree Management
Treeherder
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: martianwars, Assigned: torroid, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(3 files, 3 obsolete files)
In a number of places in Treeherder's code, I've written decision_task_id in the camel case notation, decisionTaskID. For the python files, it may be a good idea to re-write them as decision_task_id instead.
Reporter | ||
Updated•8 years ago
|
Depends on: 1282906
Keywords: good-first-bug
Assignee | ||
Comment 1•8 years ago
|
||
Hi I would like to work on this bug, please could you point me in the direction where the code needs to be changed.
Reporter | ||
Comment 2•8 years ago
|
||
Hey Glenn! You could work on this most definitely. Have you downloaded the Treeherder code? Here it is https://github.com/mozilla/treeherder. You can read the docs on installation https://treeherder.readthedocs.io/installation.html Once you have TH setup, we can get started :)
Assignee | ||
Comment 3•8 years ago
|
||
Hi! I have cloned the repo but Im stuck while installing prerequisites, 'vagrant up' command is giving error. As soon I resolve it, I'll get back.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(kalpeshk2011)
Comment 4•8 years ago
|
||
If you pastebin the error seen whilst running `vagrant up`, I'll make some suggestions :-)
Assignee | ||
Comment 5•8 years ago
|
||
https://pastebin.mozilla.org/8905429
Comment 6•8 years ago
|
||
When reporting issues like this, it's best practice to include: * the exact OS types/versions as well as any other software involved (eg in this case OS type, OS version, virtualbox version, vagrant version) * the exact steps performed * the full error message (not just the last line) If you haven't already seen it, http://www.chiark.greenend.org.uk/~sgtatham/bugs.html is a good read btw :-)
Assignee | ||
Comment 7•8 years ago
|
||
Sorry for the inconvenience. OS : Ubuntu 16.04.1 LTS 64-bit Vagrant version : 1.8.6.dev Virtual box : VirtualBox Graphical User Interface Version 5.0.24_Ubuntu r108355 (had manually installed) Steps performed: 1. Installed Virtual box using apt-get virtualbox 2. Installed rvm followed by ruby and dependencies 3. Installed bundler 4. Cloned vagrant.git 5. Then, used command 'vagrant up' Got Error -> https://pastebin.mozilla.org/8905433
Flags: needinfo?(emorley)
Comment 8•8 years ago
|
||
I wouldn't install vagrant manually, use the package on https://www.vagrantup.com/downloads.html The error message does actually suggest this: """ You appear to be running Vagrant outside of the official installers. Note that the installers are what ensure that Vagrant has all required dependencies, and Vagrant assumes that these dependencies exist. By running outside of the installer environment, Vagrant may not function properly. To remove this warning, install Vagrant using one of the official packages from vagrantup.com. """
Flags: needinfo?(emorley)
Assignee | ||
Comment 9•8 years ago
|
||
Hi! It worked. I am able to run the server. As per the treeherder installation guidelines I used the following command: vagrant ~/treeherder$ celery -A treeherder worker -B --concurrency 5 Please could I know what this command is doing, and should I let it run?
Flags: needinfo?(emorley)
Reporter | ||
Comment 10•8 years ago
|
||
Glenn, For all practical purposes, I think this would be a better approach https://treeherder.readthedocs.io/installation.html#ingesting-a-single-push-at-a-time, especially in the case of this bug. To get a recent push (last 4 hours), you can take a revision from https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound. Look for the codes on the far left of the screen just under the date. Make sure the date / time is not older than 4 hours.
Flags: needinfo?(kalpeshk2011)
Assignee | ||
Comment 11•8 years ago
|
||
I used single push. I really sorry, but I'm new to all this, please could you explain me how to take the revision.
Flags: needinfo?(kalpeshk2011)
Reporter | ||
Comment 12•8 years ago
|
||
Not an issue, I took 1 day to figure this out :) Try to search for "bfd9274acb82" on https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound. Then try to read my description above. Make sure you do this in the next few hours. Next try clicking on the date above "bfd9274acb82". You will open just one push having "bfd9274acb82" in the URL. Revisions can be used to uniquely identify patches / pushes. This is the link you should get on pressing the date https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bfd9274acb82b0c829f20c6d8143520ebe95d5ca. As you can see, the revision is in the query string
Flags: needinfo?(kalpeshk2011)
Updated•8 years ago
|
Flags: needinfo?(emorley)
Assignee | ||
Comment 13•8 years ago
|
||
Thanks. I got it. How do I proceed further?
Reporter | ||
Comment 14•8 years ago
|
||
So now you need to find all instances in the python code where decisionTaskId has been named incorrectly. Make it decision_task_id. Make sure you don't change the JS code. To test this, you will need to test the "Add New Jobs" feature. You can read about it on https://wiki.mozilla.org/ReleaseEngineering/TryServer#How_to_push_to_try. Make a Pull Request, I'll tell you more about how you can go about testing it :)
Comment 15•8 years ago
|
||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
Hi, I have made a pull request. I hope I'm doing it correctly, if not, please do tell me where I have gone wrong.
Flags: needinfo?(kalpeshk2011)
Assignee: nobody → glenn28f
You could likely put both commits into a single pull request, that way everything can be handled more easily.
Which would likely end up using something like this: git checkout glenn124f-patch-1 git cherry-pick 29a266dad7f1eebe48e3dacae4b530540b6829ce git push glenn124f-patch-1 Then you could mark the attachment in the bug for patch 2 as obsolete.
Comment 20•8 years ago
|
||
Assignee | ||
Comment 21•8 years ago
|
||
I've put it in one request now.
Attachment #8785711 -
Attachment is obsolete: true
Attachment #8785713 -
Attachment is obsolete: true
Reporter | ||
Comment 22•8 years ago
|
||
Comment on attachment 8785716 [details] [review] [treeherder] glenn124f:master > mozilla:master Good try! Left a few comments on Github. From next time, 1. Set me a review request rather than a need info. Click on Details next to your automatically generated PR attachement on top of the comments in the bug. 2. Try doing "git grep <regex>" to find all instances of a certain block of code
Flags: needinfo?(kalpeshk2011)
Attachment #8785716 -
Flags: review-
Reporter | ||
Comment 23•8 years ago
|
||
Armen, this will need to be changed after this bug https://github.com/mozilla/pulse_actions/blob/master/pulse_actions/handlers/treeherder_add_new_jobs.py#L50
Flags: needinfo?(armenzg)
Comment 24•8 years ago
|
||
Updated•8 years ago
|
Attachment #8785716 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8785964 -
Flags: review?(kalpeshk2011)
Reporter | ||
Comment 25•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master Closer, but you need to reflect the changes in the API call as well, in the JS code.
Attachment #8785964 -
Flags: review?(kalpeshk2011) → review-
Assignee | ||
Comment 26•8 years ago
|
||
So, would I have to make the changes in these 2 files? : 1. ui/js/models/resultsets_store.js 2. ui/js/controllers/jobs.js
Flags: needinfo?(kalpeshk2011)
Reporter | ||
Comment 27•8 years ago
|
||
Glenn, Just ui/js/models/resultsets_store.js would do, since we just want to change the HTTP request parameter and not the variable name convention in JS code
Flags: needinfo?(kalpeshk2011)
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master I hope this is right.
Attachment #8785964 -
Flags: review- → review?(kalpeshk2011)
Comment 29•8 years ago
|
||
(In reply to Kalpesh Krishna [:martianwars] from comment #23) > Armen, this will need to be changed after this bug > https://github.com/mozilla/pulse_actions/blob/master/pulse_actions/handlers/ > treeherder_add_new_jobs.py#L50 What should it change to?
Flags: needinfo?(armenzg)
Reporter | ||
Comment 30•8 years ago
|
||
We will need something like https://github.com/mozilla/pulse_actions/blob/master/pulse_actions/handlers/treeherder_add_new_jobs.py#L41-L44 with the two names being "decision_task_id" and "decisionTaskID" until the changes have been fully merged to Treeherder.
Reporter | ||
Comment 31•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master I'm sorry for not being clear earlier, but you just need to change the API's parameter, not all the variables in the JS file. So just change the "decisionTaskID" in the quotes, as that will be the API parameter. https://github.com/mozilla/treeherder/blob/master/ui/js/models/resultsets_store.js#L348
Attachment #8785964 -
Flags: review?(kalpeshk2011) → review-
Comment 32•8 years ago
|
||
Attachment #8787629 -
Flags: review?(kalpeshk2011)
Reporter | ||
Comment 33•8 years ago
|
||
Comment on attachment 8787629 [details] [review] Bug 1286897 - Support decisionTaskID & decision_task_id for adding new jobs I think after addressing emorley's comment, we are good to merge this.
Attachment #8787629 -
Flags: review?(kalpeshk2011) → feedback+
Assignee | ||
Updated•8 years ago
|
Attachment #8785964 -
Flags: review- → review?(kalpeshk2011)
Reporter | ||
Comment 34•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master I'm sorry, you are still not quite there. Have a look at https://github.com/mozilla/treeherder/pull/1819/files. You need to capitalize the D and write it like geckoDecisionTaskID, what it originally was. Next time, look at the diff (https://github.com/mozilla/treeherder/pull/1819/files) and see if the only changes are "decisionTaskID" --> "decision_task_id" in the JS file. This might be getting irritating for you, but it's just making the commit perfect :) We'll try to get you a more interesting Treeherder bug next time around!
Attachment #8785964 -
Flags: review?(kalpeshk2011) → review-
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master No no, its not irritating. I hope you're not irritated by me missing on small stuff here and there. I'll try to be more alert while changing code henceforth, and thanks for being so patient with me. I hope this one is okay.
Attachment #8785964 -
Flags: review- → review?(kalpeshk2011)
Reporter | ||
Comment 36•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master This is looking good! I'm sorry, I forgot to mention that you need to make changes in https://github.com/mozilla/treeherder/blob/master/ui/js/models/resultset.js#L205 as well. There are 2 APIs using "decisionTaskID", both need to be changed
Attachment #8785964 -
Flags: review?(kalpeshk2011) → feedback+
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master I hope this is okay
Flags: needinfo?(kalpeshk2011)
Attachment #8785964 -
Flags: review?(kalpeshk2011)
Reporter | ||
Comment 38•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master This is looking great! However, we should wait for https://bugzilla.mozilla.org/show_bug.cgi?id=1301505 to merge before we can test it. emorley, could you push this to treeherder-stage once https://bugzilla.mozilla.org/show_bug.cgi?id=1301505 is merged to mozilla-central?
Flags: needinfo?(kalpeshk2011)
Attachment #8785964 -
Flags: review?(kalpeshk2011)
Attachment #8785964 -
Flags: review+
Attachment #8785964 -
Flags: feedback+
Comment 40•8 years ago
|
||
Happy to test on stage once that bug merges - needinfo when it's ready to happen (otherwise I'm going to have to continually check back). This should also have review from either Armen or Cameron.
Flags: needinfo?(emorley)
Comment 41•8 years ago
|
||
Glenn, if you could also squash the 9 commits in the PR into one, and give it an appropriate commit message (see previous commits in the repo for examples), that would be great :-)
Assignee | ||
Comment 42•8 years ago
|
||
I finding it difficult to combine all the commits into a single one. Would it be okay to close the pull request and open a new one with just one commit?
Comment 43•8 years ago
|
||
It's a skill worth learning :-) See: https://davidwalsh.name/squash-commits-git
Assignee | ||
Comment 44•8 years ago
|
||
The issue I'm having is as follows: I had forked the treeherder repo into my git account, and had made the changes on the master branch of the forked repo on github itself and also made the pull requests through github. Now, I have cloned the forked treeherder repo (the one in my account) onto my system locally. When I do "git log" it shows me only 4 commits, when there are 9.
(In reply to Glenn from comment #44) > When I do "git log" it shows me only 4 commits, when there are 9. `git log` will only show one screen's worth of log info at a time, which in the default git bash shell for me, shows approximately four commits' worth of information. To show more, you need to press/hold the enter key to start paging in older logs. In this case, though, you should be able to rebase/squash what you want doing the following: > git checkout master > git rebase -i HEAD^^^^^^^^^ And then following the rest of the tutorial from the link in Ed's post.
(In reply to Wes Kocher (:KWierso) from comment #45) (Note, there are nine '^'s in that, meaning you want to go back and rebase the nine most recent commits from the HEAD of the branch you're on.)
Assignee | ||
Comment 47•8 years ago
|
||
I've done that and I've reached at this point : https://pastebin.mozilla.org/8910182 Should I remove all those 9 commit messages and replace by just one commit message?
(In reply to Glenn from comment #47) > I've done that and I've reached at this point : > https://pastebin.mozilla.org/8910182 > > Should I remove all those 9 commit messages and replace by just one commit > message? Yeah, one single commit message explaining what the newly-squashed patch as a whole is doing. Something like "Bug 1286897 - Change references of decisionTaskID to decision_task_id" would be a good start. Ed or Kalpesh can nitpick beyond that if needed.
Assignee | ||
Comment 49•8 years ago
|
||
Ive come across this error: error: There was a problem with the editor 'vi'. Please supply the message using either -m or -F option. Could not apply 92f33d2a50f0912080045299c75186de3e03aae0... Update resultset.js
Assignee | ||
Comment 50•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master I've done it. Hope its okay. Please do tell me if the commit message is okay.
Attachment #8785964 -
Flags: review?(kalpeshk2011)
Attachment #8785964 -
Flags: review?(emorley)
Attachment #8785964 -
Flags: review+
Comment 51•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master Looks good to me (apart from the commit message missing the bug number, but if needs be we can fix on merge), however I'd like Cameron to confirm whether the json schema should have the version number bumped?
Attachment #8785964 -
Flags: review?(emorley) → review?(cdawson)
Reporter | ||
Comment 52•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master This looks great! We really ought to check it once on stage though. I'll happily check with the pulse queue once this has been pushed to stage.
Attachment #8785964 -
Flags: review?(kalpeshk2011) → review+
Assignee | ||
Comment 53•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master Added the bug number to the commit message.
Attachment #8785964 -
Flags: review?(emorley)
Attachment #8785964 -
Flags: review?(cdawson)
Attachment #8785964 -
Flags: review-
Attachment #8785964 -
Flags: review+
Comment 54•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master Please leave the reviewer as-is (comment 54).
Attachment #8785964 -
Flags: review?(emorley)
Attachment #8785964 -
Flags: review?(cdawson)
Attachment #8785964 -
Flags: review-
Comment 55•8 years ago
|
||
sorry, comment 51
Assignee | ||
Comment 56•8 years ago
|
||
Oh. Sorry about that.
Reporter | ||
Comment 57•8 years ago
|
||
This is looking great! wlach pushed it to Treeherder Stage and I could successfully produce a pulse message with the correct inputs and parameters. Good job Glenn!
Reporter | ||
Updated•8 years ago
|
Attachment #8792859 -
Flags: review?(emorley)
Updated•8 years ago
|
Attachment #8792859 -
Flags: review?(emorley) → review?(cdawson)
Assignee | ||
Comment 58•8 years ago
|
||
Thats great. Thanks :-).
Comment 59•8 years ago
|
||
Comment on attachment 8792859 [details]
Screenshot from 2016-09-20 16:42:28.png
I'll go ahead and give this an r+, I gave it a once over before pushing to stage and it looked fine.
Attachment #8792859 -
Flags: review?(cdawson) → review+
Comment 60•8 years ago
|
||
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/dc3d9474d96fc89de12851bb7985c2203e242a36 Bug 1286897 - Change references of decisionTaskID to decision_task_id (#1819)
Comment 61•8 years ago
|
||
Thanks for the contribution Glenn (and thank you :martianwars for the mentoring!)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 62•8 years ago
|
||
Comment on attachment 8785964 [details] [review] [treeherder] glenn124f:master > mozilla:master Thank you Will. My apologies for my lateness on reviewing this. But looks good to me, too.
Attachment #8785964 -
Flags: review?(cdawson) → review+
Assignee | ||
Comment 63•8 years ago
|
||
Thanks to all the mentors for being patient and helping me solve the bug.
Reporter | ||
Comment 64•8 years ago
|
||
Will, do you have another simple bug for Glenn? Or a larger project which can come under Quarter of Contribution #4 ? Glenn, would you be interested in continuing Treeherder contribution?
Flags: needinfo?(wlachance)
Flags: needinfo?(glenn28f)
Comment 65•8 years ago
|
||
(In reply to Kalpesh Krishna [:martianwars] from comment #64) > Will, do you have another simple bug for Glenn? Or a larger project which > can come under Quarter of Contribution #4 ? Glenn, would you be interested > in continuing Treeherder contribution? We should be able to find something. The best route forward is probably to get in touch with us on irc.mozilla.org #treeherder.
Flags: needinfo?(wlachance)
Flags: needinfo?(glenn28f)
You need to log in
before you can comment on or make changes to this bug.
Description
•