Skip to content

first draft to merge the list of machines in a page#20

Open
sblondon wants to merge 32 commits intoDebian:masterfrom
sblondon:master
Open

first draft to merge the list of machines in a page#20
sblondon wants to merge 32 commits intoDebian:masterfrom
sblondon:master

Conversation

@sblondon
Copy link

This pull request add 2 urls usables to display data about the machines:
/machines/
/machines/name
They are mapped on 2 differents views and templates.

The code is minimal. I needed to change the LDAP protocol from ldaps to ldap. With ldaps, the request were refused. This modification is not included into the patch.

The html code does not include the mandatory code to be valid (html, body tags). I didn't add it to simplify this code review. I plan to add it later.

After writing it, I think it would be better to include this code and templates in a specific directory (like 'web' or 'www') to separate it from the rest of the application. What do you think about it?

What do you think about the code of the pull/request too?

@pabs3
Copy link
Member

pabs3 commented May 21, 2016

I prefer these URLs instead:

/machines
/machine/draghi

Please merge the cleaning commit into the first commit.

It would be nice if the templates matched what we have already.

ldaps is supported by db.debian.org, not sure why you got an error.

I don't know django very well, but ISTR that it has a way to map views
to URLs in templates so you don't have to hard-code URLs in hrefs.

@sblondon
Copy link
Author

  • the last commit is rebased (fixed up)
  • mapping views -> url in templates: done
  • add the rest of the current template: todo (I plan to do it the week.)

Are you sure about the '/machine/name' instead of the plural version ('/machines/name')?
I see more example for the latest one (for example http://www.vinaysahni.com/best-practices-for-a-pragmatic-restful-api or http://www.restapitutorial.com/lessons/restfulresourcenaming.html).
The django tutorial provide another way to do it (/machines and /machines/machine/name).
If you prefer your way, I will do it like you prefer.

@pabs3
Copy link
Member

pabs3 commented May 25, 2016

On Tue, 2016-05-24 at 17:42 -0700, sblondon wrote:

Are you sure about the '/machine/name' instead of the plural version
('/machines/name')?

I prefer it but either way works.

I see more example for the latest one (for example
http://www.vinaysahni.com/best-practices-for-a-pragmatic-restful-api
or http://www.restapitutorial.com/lessons/restfulresourcenaming.html)

Those are about REST APIs, which is not what this patch is about.

The django tutorial provide another way to do it (/machines and
/machines/machine/name).

That is even more ugly :)

bye,
pabs

http://bonedaddy.net/pabs3/

@sblondon
Copy link
Author

In the new commits :

  • the detail URL is /machine/name.
  • the structure of the pages are the same as in production. The static files (js and css) are directly linked to the production version. I plan to copy them into a media/ directory:
    -- media/vendors/ for jquery and other libraries
    -- at the root of media/ the files specific to the website (like debian.css for example)

Do you agree with it?

It seems that some data are not available in common.models.DebianHost (or __LdapDebianServer) so I can't insert them into the pages. Did I missed something?

@pabs3
Copy link
Member

pabs3 commented May 29, 2016

On Wed, 2016-05-25 at 16:35 -0700, sblondon wrote:

the detail URL is /machine/name.

Thanks.

I plan to copy them into a media/ directory: -- media/vendors/ for
jquery and other libraries

Please don't create embedded code copies, we will have ud depend on the
Debian packages of any external libraries needed, as we do for the
existing site that uses userdir-ldap-cgi.

https://wiki.debian.org/EmbeddedCodeCopies
https://anonscm.debian.org/cgit/mirror/userdir-ldap-cgi.git/

It seems that some data are not available in common.models.DebianHost
(or __LdapDebianServer) so I can't insert them into the pages. Did I
missed something?

Which data are you talking about? I guess you will need to add the data
to those classes.

Some more comments:

These files contain trailing whitespace:

src/templates/machines/all.html
src/templates/machines/details.html

I would use os.path.join() instead of + "/ 

The way the urlpatterns stuff is looks weird to me, I especially wonder
if it works when passing parameters.

ROOT_URLCONF shouldn't have two definitions.

I would call all_machines list instead.

bye,
pabs

https://wiki.debian.org/PaulWise

@sblondon
Copy link
Author

There are still a lot of stuff to do but there are some news:

Please don't create embedded code copies, we will have ud depend on the
Debian packages of any external libraries needed, as we do for the
existing site that uses userdir-ldap-cgi.

https://wiki.debian.org/EmbeddedCodeCopies
https://anonscm.debian.org/cgit/mirror/userdir-ldap-cgi.git/

I found the debian/links file for userdir-ldap-cgi and copy-pasted it with 'ud' as name. It probably not sufficient but it's a start.

It seems that some data are not available in common.models.DebianHost
(or __LdapDebianServer) so I can't insert them into the pages. Did I
missed something?

Which data are you talking about? I guess you will need to add the data
to those classes.

Yes, it was my conclusion. It' probably my next task, competing with understanding why ldaps protocol raises an error (and not ldap).

These files contain trailing whitespace:

src/templates/machines/all.html
src/templates/machines/details.html

Fixed.

I would use os.path.join() instead of + "/

Fixed.

The way the urlpatterns stuff is looks weird to me, I especially wonder
if it works when passing parameters.

Yes, it looks weird due to the difference from '/machines' and '/machine/name'. But perhaps you talk about something else?

ROOT_URLCONF shouldn't have two definitions.

Fixed.

I would call all_machines list instead.

Do you talk about the file src/templates/machines/all.html?

I added transformation from string like '[[gluck.debian.org]]' to '<ahref="gluck.debian.org">gluck.debian.org' too.

@sblondon
Copy link
Author

Is it ok to use javascript for sorting the table of all machines or should I do it server-side?

@sblondon
Copy link
Author

Additionals info about the machines are added.

It seems the LDAP is not available under TLS layer:

stephane@foehn:~/src/ud/src$ telnet db.debian.org 389
Trying 82.195.75.106...
Connected to db.debian.org.
Escape character is '^]'.
^C)^[)
Connection closed by foreign host.

stephane@foehn:~/src/ud/src$ telnet db.debian.org 686
Trying 82.195.75.106...
Trying 2001:41b8:202:deb:1a1a:0:52c3:4b6a...
telnet: Unable to connect to remote host: Network is unreachable

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.

2 participants

Comments