Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Initial version of audio API for #584 #1132

Merged
merged 1 commit into from
Mar 23, 2016

Conversation

hkuhn42
Copy link
Contributor

@hkuhn42 hkuhn42 commented Mar 3, 2016

Initial version of audio API for #584

See Issue #584
Also-By: @kdavis-mozilla
Signed-off-by: harald.kuhn@gmail.com

@hkuhn42 hkuhn42 closed this Mar 5, 2016
@kaikreuzer
Copy link
Contributor

Hey, why did you close it before I could review...?

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 5, 2016

It failed the initial eclipse validation due to @kdavis-mozilla missing CLA and because i forgot to commit with -s, now i am trying not find out how to fix this 😞

@kaikreuzer
Copy link
Contributor

You must mention him with his e-mail address under which he signed the CLA, not hit github id.
But that is no reason to close the PR - anyhow, you will have to squash all commits into one once we are all ok to merge it. It is good enough to make sure for the squashed commit that all CLAs are then correctly validated.

@kaikreuzer kaikreuzer reopened this Mar 5, 2016
@kdavis-mozilla
Copy link

According to validate I have a valid CLA under kdavis@mozilla.com

Bundle-ClassPath: .
Import-Package: org.slf4j
Service-Component: OSGI-INF/*,
OSGI-INF/audioContext.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this line

@kaikreuzer
Copy link
Contributor

Thanks @hkuhn42, I am done with my first review!

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 8, 2016

@kaikreuzer thanks for the review. It looks like @kdavis-mozilla beat me to adressing your issues 😄

@kdavis-mozilla
Copy link

@kaikreuzer Any idea why my IP validation fails. The validate page says there is a valid CLA for kdavis@mozilla.com and I've signed off for both my commits[1][2].

@kaikreuzer
Copy link
Contributor

@kdavis-mozilla Just ignore it. Once the PR is squashed, it might work. If not, I can still merge it when I manually check that the CLA is there (and I can confirm that it is!)

@kaikreuzer
Copy link
Contributor

@maggu2810
Copy link
Contributor

@kaikreuzer
Copy link
Contributor

@maggu2810 - I have learned ;-) This is the second link in #1132 (comment).

@maggu2810
Copy link
Contributor

@kaikreuzer Sorry, "wer lesen kann, ist klar im Vorteil"

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 10, 2016

@maggu2810 @kaikreuzer
Please review my changes to karaf and runtime feature.xml and io bundle pom.
Thanks

@kdavis-mozilla
Copy link

@hkuhn42 Literally just finished the squash in my local repo :-)

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 10, 2016

@kdavis-mozilla: Perfect timing 😃

The only thing i do not understand is whats causing the ip-validation to fail.

@@ -16,6 +16,7 @@
<packaging>pom</packaging>

<modules>
<module>org.eclipse.smarthome.io.audio</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole file should use spaces for indentation. So please keep the current scheme.

@maggu2810
Copy link
Contributor

@kaikreuzer Currently the io.voice and now io.audio is part of "bundles" and Karaf feature "esh-base".
Should we move this to "extensions" and create two separate Karaf features?
As long as io.voice is already a bundle / base thing we could do this in a PR after this one, if we would like to keep it "simple" and to get this one ready for merge.

@kaikreuzer
Copy link
Contributor

@hkuhn42 Literally just finished the squash in my local repo :-)

Ok, and now we need the squash on this PR as well, please!

@maggu2810 These bundles are imho still "base" bundles, since they define the interface and core framework parts. Bundles that implement sources/sinks etc. will be considered as extensions then.

@kaikreuzer
Copy link
Contributor

@hkuhn42, still waiting for you to squash the commits - let us know, if you need any assistance!

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 16, 2016

@kaikreuzer sorry there was a misunderstanding, I thought @kdavis-mozilla would push his squashed commit into the repo. However I tried to do it today.
As i am quite new to git and never did this, help would be really appreciated. I tried to do a git rebase -i today but i ended up having all other smarthome repo commits in the editor as well. Looks like it was a bad idea to try and keep my fork in sync with the smarthomne repo. I am not sure what to do now. How (which command in interactive mode) should i handle all non related commits?
Thanks in advance!

@kdavis-mozilla
Copy link

@hkuhn42 I saw the same problem when I tried to squash from your fork. There are so many commits of other people interspersed between the commits for the audio API I just gave up after trying the standard way of squashing.

What I did was simply take the URL of this page

https://github.com/eclipse/smarthome/pull/1132

add .diff to the end to get the URL

https://github.com/eclipse/smarthome/pull/1132.diff

Visiting this URL gives a diff between your fork and master.

What I then did to create another for of this repo and apply a patch created from the diff

https://github.com/eclipse/smarthome/pull/1132.diff

to the new fork.

This commit could then be used as the final pull request here as it contains all the relevant changes. For example the result of this process is in my fork here[1]

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 20, 2016

@kdavis-mozilla thanks for the tip. However i cannot create a second fork of smarthome project via github. If i delete the current fork, all work including this pr is lost so this seems not to be an option.

I tried to squash the commits several times not but i could not find the right approach. Any other suggestions?

@kdavis-mozilla
Copy link

@hkuhn42 I don't know if it's an option, but we could kill this pull request and you or I could use the technique I suggested above and make a new pull request.

@maggu2810
Copy link
Contributor

Without following the whole discussion, what are your problems with Git?
Could I help you with rebase?

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 20, 2016

@kdavis-mozilla i am starting to think that this is the only way out, however i am stubborn so i will try one last time 😄
@maggu2810 yes, any help is appreciated! Sorry, i am a git newbi.
I made the mistake to sync my feature branch with the smarthome main repo prior to trying to squash my commits. Now i have a lot of commits when i try to to rebase -i (b2064cb). I tried to reword the first commit, fixup the others of our feature and just pick the non related ones but i end up having numerous merge conflicts and after i try fix them there are other problems (not push as there is no fast forward etc.).

@maggu2810
Copy link
Contributor

maggu2810 commented Mar 20, 2016 via email

@maggu2810
Copy link
Contributor

What is the branch you want to rebase + squash? This one?

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 20, 2016

@maggu2810 yes, i tried to squash the commits from the audio branch of hkuhn42/smarthome.

Bug: eclipse-archived#584
Also-By: Kelly Davis <kdavis@mozilla.com>
Signed-off-by: Harald Kuhn <harald.kuhn@gmail.com>
@maggu2810
Copy link
Contributor

WDYT about this one:
master...maggu2810:audio

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 20, 2016

Looks good to me! How do i get it into this PR?

@maggu2810
Copy link
Contributor

I assume you named your remote "hkuhn42". You have to change that if it differs.

First you could create a backup of your current branch:

git push hkuhn42 hkuhn42/audio:refs/heads/audio-backup

Add my remote to your local git clone

git remote add maggu2810 git@github.com:maggu2810/smarthome

Fetch from remote

git fetch maggu2810

Push my branch to yours (force)

git push -f hkuhn42 maggu2810/audio:audio

@hkuhn42
Copy link
Contributor Author

hkuhn42 commented Mar 20, 2016

@maggu2810 Ok, had to create a new ssh key. After that it worked like a charm. Thank you so very much!!!

@kaikreuzer
Copy link
Contributor

Thank you all - looks good now!

@kaikreuzer kaikreuzer merged commit c97516a into eclipse-archived:master Mar 23, 2016
cdjackson pushed a commit to cdjackson/smarthome that referenced this pull request Mar 28, 2016
@kaikreuzer kaikreuzer modified the milestone: 0.8.0.b5 May 22, 2016
@mstormi mstormi mentioned this pull request Dec 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants