Pending chronometer.sh changes #15
Conversation
Consume JSON output of chronometer.sh Relies on [this pull](pi-hole/pi-hole#193) being merged.
Consume JSON output of chronometer.sh Relies on [this pull](pi-hole/pi-hole#193) being merged.
|
That link is broken |
Which one? |
|
In the first comment (index.php and api.php changed to consume the changes made to chronometer.sh in this pull.) |
|
Yeah, I guess you can't use markdown in commits. |
|
When you set up your PR's I think you can set the origin to be a new branch instead of master, that would make it a little easier to test the code and prevent breakage. Merging to master is kind of a done deal. (Reverts are a pain in the posterior, lol.) |
|
I couldn't see the option to do so.. I'll gladly do another if that is the case... |
|
Before you start working, you can create a new branch and change to it. Then any work you do will be put on that branch. When you're done, make a pull request to add a branch here. (I think that's how it works). |
|
Just type a new branch name in the box. |
|
Yeah, did that, but it didn't give me an option to create a new one in the base fork. |
|
Hmm... Let me do some research. |
|
Can you grab a screenie of what you see when you do a PR? |
|
I made changes to TEST NEW BRANCH, so there should have been things to compare |
|
Can you click on the |
|
Yeah, I typed in "TEST NEW BRANCH" to the base branch box, and then when you then click on the branch in the drop down it redirects you to the page shown in my first screenshoy. |
|
Oops, just re-read that... Hmmm... Have to see whats up with that. |
|
Is the box blank because you're trying to submit a PR with no changes? Try doing that with a PR that has some changes to merge and check if the box goes green? |
|
Nope, the Create Pull request button is greyed out.
If you check my test fork you'll see I made changes to the readme |
|
So this PR is for a new branch called "PendingChronometerChanges"? If I merge it, it should create the new branch? Is that what you are saying? |
|
The PR is to merge my branch "PendingChronometerChanges" into the main "master" |
|
Oh yes, I see now. I read it wrong. |
|
Yeah, there is some confusion with the branching. As far as I can see, there is no way to duplicate a branch to the upstream without the owner creating said branch. |
|
I'll have to do some more research to see how a PR can be requested for a branch to be created on the base repo. That way we aren't merging testing code to master and instead to a new branch on origin. |
|
I'm pretty sure you're supposed to merge locally and test before merging on github. That way you can test changes without destroying master. |
|
I think the idea is that the owner creates a few branches (master, v1, v1-dev) and forks can go off on their own (branching off of v1-dev to make a new feature in new branch feature). After creating the feature, they make a pull request (pr from fork's feature into origin's v1-dev). Their changes get put into the inherently unstable branch, which every once in a while gets it's features merged into v1. |
|
@Mcat12 has it right. |
|
@Mcat12 I think that's probably the good workflow. @jacobsalmela could create a |
|
Agreed. I wasn't expecting so much support for the project so I really need to learn more |
|
It's the Git to GitHub integration that I'm trying to figure out. Local git repositories are a breeze for me, but when you add in GitHub as an upstream remote things get cloudy for me... |
|
We quite often have arguments about it at work, though we use SVN there, which is slightly different. One day I'll convince them to switch to git. I much prefer it. |
|
@PromoFaux, I'm just trying to catch up on the activity from today. Does this PR also fix Pi-hole #191? |
|
😄 Good thing I got to git from Github (Except then again, I sometimes have the opposite problem...). |
|
@jacobsalmela It fixes the issues I could see with Pi-hole #191 , but I was not able to replicate the original issue. |
|
That issue is with the original adminlte. The correct issue is on another repo, so you need to directly link to it. |
|
@Mcat12 spotted that and fixed it with a sneaky ninja-edit! |
|
As an aside, it's kind of annoying that adminlte has such a huge fork list. Github won't let me see the branch network from Graphs -> Network. |
|
Provided everyone else is happy with it, it's safe to merge to master @jacobsalmela |
|
Everything looks good. |
Pending chronometer.sh changes
index.phpandapi.phpchanged to consume the changes made to chronometer.sh in this pull.