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

Webui changes #278

Merged
merged 7 commits into from Dec 8, 2011

Conversation

Projects
None yet
3 participants
@tardyp
Copy link
Member

commented Nov 7, 2011

dustin,
this is an updated version of my webui changes (cookie based authentication, and forcesched)
Those are interdependant, and thus sent in the same branch, as well as with some minor ui tweaks.

This has run in prod for several month now, 15 maintainers are using it in a daily basis, and do not have any known bugs anymore.

@tardyp

This comment has been minimized.

Copy link
Member Author

commented Nov 7, 2011

updated with comments handled from pull:
#247

@djmitche

This comment has been minimized.

Copy link
Member

commented Nov 14, 2011

This is looking great. A few comments:

First, merged to trunk, this fails with many errors like

[ERROR]
Traceback (most recent call last):
  File "/home/dustin/code/buildbot/t/buildbot/master/buildbot/test/unit/test_status_web_authz_Authz.py", line 184, in test_needUserForm_http_False
    assert not z.needUserForm('forceBuild')
exceptions.AttributeError: 'Authz' object has no attribute 'needUserForm'

buildbot.test.unit.test_status_web_authz_Authz.TestAuthz.test_needUserForm_http_False

can you fix those up?

The new scheduler will need unit tests. It'd be great to name it ForceScheduler, too, for consistency with SingleBranchScheduler and AnyBranchScheduler.

In the docs, the .. py:class: is for use when documenting a class's interface, rather than its use in the configuration. Also, the API documentation will be going away, and was never intended for user reference - please document the parameters and classes used with ForceSched in the sphinx documentation directly. Examples are great, but they do not replace the need for comprehensive documentation.

There are a number of "todo" items in this series of commits - please either complete them, or remove the TODO items. There's also at least one debug print (in forceWithWebRequest)

I suspect that the if/elif with isinstance calls could be handled better through a generic method in the parent class - maybe just a more generic definition of parse()?

I'm not sure how wise it is to cross the namespaces of properties and request arguments (the properties dict in forceWithWebRequest).

The scheduler should probably use the addBuildsetForSourceStamp method, rather than calling the master's method directly, just for consistency.

As for the Auth/Authz changes, I like the design quite a bit, although it does not allow me to login - entering a valid username/password brings me to a new page with the same login form. An invalid username/password will take me to the authfail page.

I've also pushed a version with some minor editing to my 'pull278' branch - https://github.com/djmitche/buildbot/commits/pull278. These edits are primarily to make pyflakes pass (removing unused imports, etc.), add some whitespace to make the code more pep8-compliant, wrap long lines, and correct some minor English errors - nothing substantial in the code.

@tardyp

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2011

First, merged to trunk, this fails with many errors like

[ERROR]
Traceback (most recent call last):
 File "/home/dustin/code/buildbot/t/buildbot/master/buildbot/test/unit/test_status_web_authz_Authz.py", line 184, in test_needUserForm_http_False
   assert not z.needUserForm('forceBuild')
exceptions.AttributeError: 'Authz' object has no attribute 'needUserForm'

buildbot.test.unit.test_status_web_authz_Authz.TestAuthz.test_needUserForm_http_False

can you fix those up?
Done, I added some more unittest for authz

The new scheduler will need unit tests.
Done
 It'd be great to name it ForceScheduler, too, for consistency with SingleBranchScheduler and AnyBranchScheduler.
Done

In the docs, the .. py:class: is for use when documenting a class's interface, rather than its use in the configuration.  Also, the API documentation will be going away, and was never intended for user reference - please document the parameters and classes used with ForceSched in the sphinx documentation directly.  Examples are great, but they do not replace the need for comprehensive documentation.
Will Do, shall I remove the doc in code? I'm a bit reluctant to have
same documentation in both rst, and code...

There are a number of "todo" items in this series of commits - please either complete them, or remove the TODO items.  There's also at least one debug print (in forceWithWebRequest)
Done

I suspect that the if/elif with isinstance calls could be handled better through a generic method in the parent class - maybe just a more generic definition of parse()?
Done

I'm not sure how wise it is to cross the namespaces of properties and request arguments (the properties dict in forceWithWebRequest).
It simplifies a lot. Actually I dont see the advantage of specializing
the request arguments (project, branch, etc). They could be plain
properties, and this would simplify a lot of code.

The scheduler should probably use the addBuildsetForSourceStamp method, rather than calling the master's method directly, just for consistency.
Done

As for the Auth/Authz changes, I like the design quite a bit, although it does not allow me to login - entering a valid username/password brings me to a new page with the same login form.  An invalid username/password will take me to the authfail page.

I fixed some bugs by running the unit tests, can you retry, and give a
config if it still fails?

I've also pushed a version with some minor editing to my 'pull278' branch - https://github.com/djmitche/buildbot/commits/pull278.  These edits are primarily to make pyflakes pass (removing unused imports, etc.), add some whitespace to make the code more pep8-compliant, wrap long lines, and correct some minor English errors - nothing substantial in the code.
I need to import them (will do from home)
Not sure how easy it will be I reworked a lot the forcescheduler to
address your issues.

Pierre

@tardyp

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2011

Dustin,
I just finished the remaining "willDo" from my last email.
Document the Parameters classes in rst, and import your formatting fixes.

All the unittest are passing, and I have made some change to the default config for use new authz and ForceScheduler.
I verified that I can login and forcebuild using this.

Should be ready for merge...

@djmitche

This comment has been minimized.

Copy link
Member

commented Nov 26, 2011

Excellent! A few remaining issues, and hopefully I won't take 6 days to review the next round (bearing in mind there was a US holiday in there..):

  • the headlines for forcing builds appear even if there is no force-build form
  • I configured things with forceBuild="auth", and ran into two problems: first, the username field does not appear in this case, but the rest of the form does. I'm using the ForceScheduler invocation from sample.cfg. More importantly, the system let me force builds without a username! Even if I set forceBuild=False, it still lets me force builds.
  • The documentation is still very developer-oriented, giving summaries of classes and interfaces without much information on how they will fit together. I would prefer that this follow the model of the other cfg-*.rst files, where the usage of each class is described, along with its parameters. The class-oriented documentation could be moved to master/docs/developer, if you'd like.
  • There's one last print in forcesched.py: print "not in buildername", builder_name ,self.builderNames. That would probably best be handled as an error when not forcing all, and ignored when forcing all.

tardyp and others added some commits Nov 3, 2011

authz: rework for cookie based session management
Instead of requiring the user to type password in each and
every form inside buildbot, we implement a simple session
manager.

Existing IAuth interface is augmented with getUserInfos()
which is used to display info about the user
/login and /logout ActionResources are added to manage login and logout

template helpers like advertiseAction() now needs to pass request, so
that authz can get the session cookie from headers

now that we can identify between auth fail and authz fail,
we also add an authz failed page in order to let the user know
whether the authentication failed of the authorization failed.

These changes happen behind the scene, there is no update in the
configuration API, and the user should just notice the change in the
UI (the login form changed)

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
web status: add alert_msg
implement a simple way for action ressources to report errors to users

if action resource returns a tuple, then it is URL, alert_msg, the message is displayed
in the next web page

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
forcesched: a scheduler for doing forced builds
The ForceSched scheduler is the way you can configure a
force build form in the web UI.

In the builder/<builder-name> web page, you will see one form per ForceSched
scheduler that was configured for this builder.

This allows you to customize exactly how you build form look like, which builder do
have a force build form (it might not make sense to force build every builders),
and who is allowed to run which force build.

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
web/builder: show more info about pending build request
In the wait for a proper nice by brid page, we display more
info about forced build requests.

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
change_macro: display change category in the UI
Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
sample.cfg: add minimal example for auth and force
force is now disabled by default, as it needs
a forced scheduler available.
We need to have the sample code at least show the basic usage

Signed-off-by: Pierre Tardy <tardyp@gmail.com>
@tardyp

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2011

On Sat, Nov 26, 2011 at 5:20 PM, Dustin J. Mitchell
reply@reply.github.com
wrote:

Excellent!  A few remaining issues, and hopefully I won't take 6 days to review the next round (bearing in mind there was a US holiday in there..):

  • the headlines for forcing builds appear even if there is no force-build form
    a simple one.. I fixed that
  • I configured things with forceBuild="auth", and ran into two problems: first, the username field does not appear in this case, but the rest of the form does.  I'm using the ForceScheduler invocation from sample.cfg.  More importantly, the system let me force builds without a username!  Even if I set forceBuild=False, it still lets me force builds.

If you are authenticated, that is the expected behavior. If you are
not authenticated you wont see any form.
If you are authenticated, it will take your authenticated username as
the build's owner. no need to type you name twice...

  • The documentation is still very developer-oriented, giving summaries of classes and interfaces without much information on how they will fit together.  I would prefer that this follow the model of the other cfg-*.rst files, where the usage of each class is described, along with its parameters.  The class-oriented documentation could be moved to master/docs/developer, if you'd like.

yes you are right, the doc was noised by the class definitions. I hope
this version is more straight forward

  • There's one last print in forcesched.py: print "not in buildername", builder_name ,self.builderNames.  That would probably best be handled as an error when not forcing all, and ignored when forcing all.

This cannot happen when not forcing all. Contract from the calling code.

Hope this is the last version. I'm very happy about it. I think this
is a huge improvement to BB...

Regards
Pierre

@djmitche

This comment has been minimized.

Copy link
Member

commented Dec 8, 2011

This looks great - the last round of patch has added significant polish! I'll get this merged, and add some notes to the release notes, too.

This is a great improvement to Buildbot, and will be enough on its own to justify a 0.8.6 release. There are a few more changes about ready to land, so once those are in place, we'll ship it!

@djmitche djmitche merged commit a98e1bc into buildbot:master Dec 8, 2011

@tomprince

This comment has been minimized.

Copy link
Member

commented on 43eb37b Mar 5, 2012

@tardyp: This commit breaks '/users', and doesn't update the relevant docs in docs/developer/webstatus.rst.

By inspection this looks like it will fix '/users', but I haven't tested it, and I'm not sure what the appropriate behavior here is.

@tardyp tardyp deleted the tardyp:webui_changes branch Dec 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.