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

Added google assistant to external ressources. #721

Merged

Conversation

Confectrician
Copy link
Contributor

Closes #690

Tested local and the maven build succeeded.
Assistant page was pulled and added.
Font matter is introduced in openhab/openhab-google-assistant#40

Signed-off-by: Jerome Luckenbach [email protected]

@Confectrician
Copy link
Contributor Author

Even if approved:

I would wait for #700 to be finished and merge directly in the new master branch then.

Wdyt @ThomDietrich and @ghys ?

@ghys
Copy link
Member

ghys commented Jun 14, 2018

I would wait for #700 to be finished and merge directly in the new master branch then.

Not the master branch, but "final" (or whatever we'll call it) :)
You can always merge and we can cherry-pick this commit afterwards.

@Confectrician
Copy link
Contributor Author

Not the master branch

I thought we manage pom.xml and the scripts still in the master.

@ghys
Copy link
Member

ghys commented Jun 14, 2018

I wasn't sure, now I'm sure it's not necessary to be kept in master ;)

@Confectrician
Copy link
Contributor Author

Confectrician commented Jun 15, 2018

Ok, then i would say we cherry pick it to the final branch and leave gh pages for now. :)

Which means:
Ready for review and merge, if everything's alright.
The font matter in usage.md isn't necessary, but of course recommended.
Fine if it joins afterwards.

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Not too familiar with these but LGTM.
Wait for the PR in the google-assistant repo to be merged before merging this one obviously ;)

Copy link
Member

@ThomDietrich ThomDietrich left a comment

Choose a reason for hiding this comment

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

👍 As you can see, it's not that difficult :)

One question though. What about openhab_google_app.png? https://github.com/openhab/openhab-google-assistant/blob/master/USAGE.md#setup--usage-on-google-assistant-app

@Confectrician
Copy link
Contributor Author

I think i have missed the picture.
Will have to adapt the pom with an include for that.

@Confectrician
Copy link
Contributor Author

Confectrician commented Jun 17, 2018

I have added an image execution (like its made for android) which works.

image

But now the usage doesnt get copied anymore.
Strange.

image

Copy to the ext folder seems to work

image

@Confectrician
Copy link
Contributor Author

An hint on this @ThomDietrich or @kaikreuzer ?

I am not a maven expert, to be honest.

@ThomDietrich
Copy link
Member

Could you update the PR with what you have?

You've added something to the middle of the file and now a step at the end of the file doesn't work anymore. Sure you did not brake the xml structure by accident?

@Confectrician
Copy link
Contributor Author

Sure you did not brake the xml structure by accident?

Vscode would have shown that.
I have testet it just now.

I have uploaded the corresponding commits.

@Confectrician
Copy link
Contributor Author

I copied it 1 on 1 from the android equivalent, but maybe i have inserted a silly mistake while trying to fix it.

@Confectrician Confectrician changed the base branch from gh-pages to final July 8, 2018 14:44
@Confectrician
Copy link
Contributor Author

I have played around with the pom again and changed orders.
I have run mvn tidy:pom but nothing helps at all.

I give up.
Maybe one of you can figure it out.

Order of execution:
Images get copied over fine.
The readme gets copied over fine

Then there happens osmethign i cant fugure out while coming from extfolder to the final location and afterwards the readme is missing.

@ThomDietrich
Copy link
Member

I'll have a look tomorrow :)

@Confectrician
Copy link
Contributor Author

Any news @ThomDietrich?

@ThomDietrich
Copy link
Member

Hey,
the cause was a bit deeper than I thought and a bunch of ideas came to my head. I've solved it the easiest way for now. Have a look: 7a5311d

The main problem was the destination of your png copy process. It's not supposed to go into the temporary folder. Turns out the groovy script didn't like that. I've added tests to catch that problem next time. The script copies a full folder at this point and thanks to your png copy the folder is already available. Boom.
Obviously the script still needs some improvement. I already invested a lot of work there. Honestly I always hoped to be able to replace the cumbersome pom.xml file by simple bash commands and the extensive groovy script by the results of eclipse-archived/smarthome#4503 - which to my surprise did to this day not attract any interest...

Maybe we could discuss further simplifications in the process now possible thanks to the divide with the new website. I did not yet look into the details of all of this.

One more thing: I don't like the structure over in https://github.com/openhab/openhab-google-assistant yet. Now that we are at it, how about we move USAGE.md to docs/ and the image file to docs/images/. As I said earlier, I'd like to establish this as the general structure for all external
addons/sources.

Anyhow, the commit linked above contains all needed changes. Please feel free to adopt them and delete the branch ;) Best!

@Confectrician
Copy link
Contributor Author

Yeah i thought we already agreed to a general structure and to move it in the google repo also.
I have made an example pr in the vscode repo.

About the scripts:
I would love to see some more comments there.
It is really hard to follow the stuff, when you have to dig into groovy and the regular script content parallel.

Maybe those scripts are simple for one who know what he is doing already.
That's not the case for everyone.
And in my opinion it should be easy to dig into this stuff for everyone who is willing to contribute.

@ThomDietrich
Copy link
Member

Yes you are sure but the actual goal should be a better solution. Both the pom and the groovy file are ridiculous... I can replace the pom file by a few bash lines when time is right. The groovy file would benefit from the issue I've linked above. Anyhow, yes next time I'm touching the groovy script, I'll add comments.

Back to the task at hand. Will you create the PR? Also do you think the image will now be picked up by the new webpage?

@Confectrician
Copy link
Contributor Author

Will you create the PR?

There are 13 commits to compare in your branch.
I can adapt this one to the working solution next days.

Also do you think the image will now be picked up by the new webpage?

@ghys do you have to adjust the ruby script for an additional image?

@Confectrician
Copy link
Contributor Author

Ok i have added the changes from #721 (comment) into this pr and opened up another one (#788) for the process addons change.

I have also directly prepared the pom-xml for the "new" docs structure.

process addons can be merged already imho.

This one should wait for the corresponding PR in google assistant integration.

@ghys
Copy link
Member

ghys commented Oct 14, 2018

So it would be the best, if you would merge the PR when its finished and directly provide a commit for the prepare docs ruby.

Okay, but I'm not yet sure I'll have time next week.

Since you are grabbing som IO folders anyway during the prepare script run, wouldn't it be the best to reflect the ecosystem here as a folder too in the final branch?
I think it would be goo to clean up the prepare docs script on mid term, with providing a better adapted file structure.

Yes, you're right. The initial goal was to avoid breaking the current flow and the old docs site, but now nothing prevents altering the structure now that we don't have to worry about Jekyll anymore.
I'd also like to migrate most of the prepare-docs script logic from the website repo to here, it could be found and launched after this repo is cloned as part of the website build.

@Confectrician
Copy link
Contributor Author

No need to hurry. This PR exists since june and has stilla dependency with the google assistant repo.

About the folders and prepare script:
Maybe we should add a Project here and split up the changes into some issues.
If we can make a roadmap and some description for this i can work on those things
and provide parallel improvements to the prepare ruby script.
I just need your help and input for collecting the needed tasks.

@Confectrician
Copy link
Contributor Author

Closed by mistake 🤦‍♂️

@Confectrician Confectrician reopened this Oct 24, 2018
@Confectrician Confectrician added stat: dependency 💥 This issue/pr has a dependency in another repo and removed external source labels Nov 21, 2018
@Confectrician Confectrician removed the stat: dependency 💥 This issue/pr has a dependency in another repo label Dec 13, 2018
Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

I'm really no expert in this, and don't have the environment to test it today, but have some doubts :)

pom.xml Outdated Show resolved Hide resolved
@Confectrician
Copy link
Contributor Author

@ghys

I did some tests (locally including the change from #788 ):
image

image
So now warnings/errors anymore

So copying throws no warnings or errors anymore.

Those are the locations with the copied content now:

image

And:
image

So indeed you seem to be right with your thoughts.
I will adapt it.

@Confectrician
Copy link
Contributor Author

Looks way better:
image

@ghys
Copy link
Member

ghys commented Jan 12, 2019

@Confectrician LGTM now! Looks like we'll able to merge this at last ;)

@Confectrician
Copy link
Contributor Author

Confectrician commented Jan 12, 2019

Not completely

I would like to have:

_addons_ios/
 |- google-assistant/
    |- readme.md
    |- images/
       |- (*.png)

since this is the orga the relative image urls in readme are expecting.

Anyway you can merge #788 already please.

ghys pushed a commit that referenced this pull request Jan 12, 2019
@Confectrician
Copy link
Contributor Author

Here we go:
image

I'll do a cleanup and push a last commt.
After that we are good to go.

I noticed that there seems to be a mistage in usage.md too and will file a pr for this in the google assistant repo.

@Confectrician
Copy link
Contributor Author

:shipit:

@ghys ghys merged commit 3207f8c into openhab:final Jan 12, 2019
@ghys
Copy link
Member

ghys commented Jan 12, 2019

🎉
I'll do the changes to the website now while I'm at it.

ghys added a commit to ghys/openhab-website that referenced this pull request Jan 12, 2019
Add hot rewrite for wrong image relative path.

Related to openhab/openhab-docs#721.
Related to openhab/openhab-docs#690.

Signed-off-by: Yannick Schaus <[email protected]>
ghys added a commit to openhab/website that referenced this pull request Jan 12, 2019
* Copy Google Assistant page + images from openhab-docs repo

Add hot rewrite for wrong image relative path.

Related to openhab/openhab-docs#721.
Related to openhab/openhab-docs#690.

Signed-off-by: Yannick Schaus <[email protected]>
@Confectrician
Copy link
Contributor Author

"Add hot rewrite for wrong image relative path."
I will fix this in the google repo with a pr.
Should i mention/inform you, when the hot rewrite can be removed?

@ghys
Copy link
Member

ghys commented Jan 12, 2019

Should i mention/inform you, when the hot rewrite can be removed?

Yes please - as a general rule. Or better, you can do a PR to remove the offending line yourself ;)

@Confectrician
Copy link
Contributor Author

Confectrician commented Jan 12, 2019

Or better, you can do a PR to remove the offending line yourself ;)

Makes totally sense.
Sometimes the simple solutions are the ones you think off at last. 😄

@Confectrician Confectrician deleted the 690-add-google-assistant-external-source branch January 12, 2019 20:01
@Confectrician Confectrician added this to the 2.x.x milestone Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants