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

Move artifact information from REVIEWING.md to PR template #278

Merged
merged 2 commits into from
Oct 29, 2021

Conversation

niconoe
Copy link
Contributor

@niconoe niconoe commented Oct 26, 2021

Description

Moved "reviewing with artifacts" information from REVIEWING.md to Pull request template, as discussed in #252.
I currently didn't copy this information in the Create tutorial article, since I felt it was a bit out of scope (pull requests are mentioned there, but not in details), and with the current system, people will see the information anyway once they create that Pull Request.

I'm happy to revisit if someone find it not clear enough from a new user point of view (any opinion @ElsLommelen or @peterdesmet?)

Related Issue

closes #252

@ElsLommelen
Copy link
Contributor

I think it is a good idea to add this information to the pull request template, in this way everyone will find it.

However, I also tried the instructions you added and I can't review the website this way, neither on Windows nor on Ubuntu. More precisely, I choose to install the Google Chrome Web Server app, an after pointing my browser to localhost (step 4), I get the main page of the tutorials site with the links to subpages (without layout, but as a reviewer I don't care too much about that), but when clicking on one of the links, I get the message entry not found: followed by the relative path of the link (e.g. /tutorials/installation/user). So from a new user point of view, I would like to be able to just review the website after executing the steps. ;-)
(The explanation seems clear, but apparently it doesn't work as expected in each situation. I never used it before, so I have no idea what could be wrong? I extracted the files to a new folder which I called tutorials, and there was a index.html in each folder, so as far as I see I did nothing wrong?)

@florisvdh
Copy link
Member

florisvdh commented Oct 26, 2021

The below (taken from a 'helpdesk' email) should work to render the site as intended (maybe if this helps you @ElsLommelen it means the explanation should be updated in the repo):

After unzipping (I chose to unzip within a pre-created directory tutorials) I have this:

$ tree -L 2 gooiweg2
gooiweg2
└── tutorials
    ├── 404.html
    ├── articles
    ├── assets
    ├── authors
    ├── categories
    ├── create_tutorial
    ├── css
    ├── favicon-32x32.png
    ├── favicon.ico
    ├── favicon.png
    ├── images
    ├── index.html
    ├── index.xml
    ├── installation
    ├── js
    ├── list_of_categories
    ├── logo.svg
    ├── robots.txt
    ├── search
    ├── sitemap.xml
    ├── tags
    └── tutorials

13 directories, 10 files

After launching the webserver app via chrome://apps, apply following configuration in 'choose folder':

afbeelding

This window must remain open for the web server to remain active.

Now go to http://localhost:8887/tutorials/ , and you should see the website.

@ElsLommelen
Copy link
Contributor

Thanks @florisvdh This helps indeed, I was a bit confused by the 2 folders named tutorials and I had pointed to the highest level folder named tutorials (as it is one level higher than the other tutorials folder). After all I think the explanation of @niconoe is clear, it it only a bit confusing to have 2 folders with the same name.

@florisvdh
Copy link
Member

Happy that this helped. However you're the second person getting stuck at the same point. @niconoe maybe you can stress this pitfall in the guideline in order to prevent further confusion?


1) On the PR page, you can find a "details" link under "checks - On PR, build the site and ...". Go there, click on the top link in the left sidebar ("Summary"), and download the generated artifact at the bottom of the page.
2) Decompress it and make sure the target directory is called 'tutorials' (you may need to rename it)
3) From the parent directory (just above tutorials), run `python -m http.server 8887`, _or_ launch the Google Chrome [Web Server app](https://chrome.google.com/webstore/detail/web-server-for-chrome/ofhbbkphhbklhfoeikjpcbhemlocgigb) and point it at the parent directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this clarifies it?

Suggested change
3) From the parent directory (just above tutorials), run `python -m http.server 8887`, _or_ launch the Google Chrome [Web Server app](https://chrome.google.com/webstore/detail/web-server-for-chrome/ofhbbkphhbklhfoeikjpcbhemlocgigb) and point it at the parent directory.
3) From the parent directory (just above the tutorials folder you created), run `python -m http.server 8887`, _or_ launch the Google Chrome [Web Server app](https://chrome.google.com/webstore/detail/web-server-for-chrome/ofhbbkphhbklhfoeikjpcbhemlocgigb) and point it at the parent directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ElsLommelen and @florisvdh:

I think it's difficult to find a good phrasing because different operating systems and tools behave differently: some will create an intermediate directory, some will not (on my machine, if I just download the zip to the Downloads directory - without manually creating a tutorial one - and double-click to unzip it, it will create a single subdirectory (pr-280-inbo-tutorials-website)). It is the one I should rename to tutorials and serve via the webserver. If I understand correctly, this is not the behaviour you observed on your computer?

I think we might need a better phrasing so people are not confused whatever system they use. I initially wrote that the directory to rename/serve is the one just one level above tutorials, but that's indeed confusing if your machine create a second "tutorials" directory directly above. Maybe we should either show the full tree in the explanation (like in @florisvdh comment above), or state that the directory to rename / serve is just one level above 404.html or robots.txt (they're at the same level than the inner tutorials directory, but their name is less confusing?)

Thanks for your opinions :)

Copy link
Contributor

@ElsLommelen ElsLommelen Oct 27, 2021

Choose a reason for hiding this comment

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

The confusing part is not the folder tutorials we create or rename ourselves, but the fact that there is already a second folder named tutorials on the same level as 404.html. So initially I thought step 3 referred to this lower level tutorials folder, and that we had to point to our self made tutorials folder (as nowhere else there is a reference to a higher level). And maybe also the wording target directory in step 2 made me point at this level rather than a higher level?

For the Google Chrome Web Server app, we had to point to the level above the tutorials we created, which was in my case the Downloads directory, in Floris' case the folder gooiweg2. So some reference in step 3 to the fact that the focal folder is the one we created or renamed, would have helped me to do the right thing, and that's why I did this suggestion. But I don't care if you prefer to describe it differently, as long as it is clear that this is the level above the one you call target directory in step 2.

Just to be sure our computers behave the same: after pointing at this higher level, in Chrome I have to descend one level again (choose for folder 'tutorials') to see the website. However, if I don't choose for this higher level at first instance, I don't get a website, just something that is not working properly (see my description in my first comment). This behavior appears to be the same in Windows and Ubuntu when using this Google Chrome Web Server app, I don't know if it is the same when using Python.

I hope this clarifies the situation and gives you some background to rephrase, @niconoe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot, indeed it helps!

@niconoe
Copy link
Contributor Author

niconoe commented Oct 28, 2021

Thanks @ElsLommelen, I now understand the issue better and I have improved the phrasing, using almost the same phrasing you suggested first, and I think it's indeed much better.

Just two small remaining points:

  1. Do we all agree the instructions are overall good/better now? (I'd like to read them again with a fresh mind, but cannot)

  2. about:

Just to be sure our computers behave the same: after pointing at this higher level, in Chrome I have to descend one level again (choose for folder 'tutorials') to see the website. However, if I don't choose for this higher level at first instance, I don't get a website, just something that is not working properly (see my description in my first comment). This behavior appears to be the same in Windows and Ubuntu when using this Google Chrome Web Server app, I don't know if it is the same when using Python.

Yes, that is the expected behaviour and this is why I wrote in point 4 to point the browser to http://localhost:8887/tutorials rather than http://localhost:8887 (so they don't need to descend one level again, and the all the internal links are correct). @ElsLommelen: can you confirm me I understood the problem fully and it works as described here for you?

Thanks a lot!

@ElsLommelen
Copy link
Contributor

Oups, my mistake... after some trials I just clicked the suggested link in Google Chrome Web Server app as 'it looked about the same except for the IP of localhost being replaced by localhost' and I more or less forgot to add \tutorials in the path. At my first trial I used the link in step 4, so with the change you made, I would have ended perfectly.

I agree that the instructions are good now. Of course I also can't look at it anymore as a person who tests it for the first time, but to me this addition seems to clarify a lot.

@florisvdh
Copy link
Member

Thanks to both of you - seems OK!

@ElsLommelen feel free to look at other PRs of Nico that you feel comfortable with. Anyway I'm afraid I can only have a look in the week of 10 Nov.

@florisvdh florisvdh merged commit dd73f08 into master Oct 29, 2021
@florisvdh florisvdh deleted the move_reviewing_information branch October 29, 2021 16:17
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.

Move information from .github/workflows/REVIEWING.md
3 participants