Skip to content
This repository has been archived by the owner on Sep 23, 2024. It is now read-only.

WIP: Remove jenkins requirement #51

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

spbnick
Copy link
Contributor

@spbnick spbnick commented May 24, 2018

This is work-in-progress of removing the requirement to have Jenkins setup and configured to run sktm.
This is not tested and likely breaks stuff at this moment.

@spbnick spbnick force-pushed the remove_jenkins_requirement branch 2 times, most recently from 641a546 to 2e481e5 Compare May 24, 2018 12:05
Move sktm.tresult to sktm.misc.tresult to fix composition, so that
modules lower in the hierarchy (e.g. sktm.jenkins) do not have to import
the module above (i.e. sktm).

Fixes cki-project#6
Instead of creating and using a "Jenkins Interface"
(sktm.jenkins.skt_jenkins), and then supplying a project name
("jobname") with every method call, create and use a "Jenkins Project
Interface" (sktm.jenkins.Project) and only supply the project name on
its creation.

This simplifes the interface, and removes a bit of code.
This also prepares for abstracting Jenkins away.
Mark sktm.jenkins.Project private methods by prepending them with double
underscore.

Concerns cki-project#12
@spbnick spbnick force-pushed the remove_jenkins_requirement branch from 2e481e5 to f35d989 Compare May 28, 2018 07:06
Spell out "patch URL list" instead of writing "patchwork" in
sktm.jenkins.Project, to make clear what's being accepted/returned.

Leave updating the Jenkins project parameter name for later.
Create a complete Jenkins project interface instance and pass it to the
watcher, instead of having the watcher create it. This prepares the
watcher to using abstract scheduler interface, instead of Jenkins
specifically.
Copy link
Contributor

@dzickusrh dzickusrh left a comment

Choose a reason for hiding this comment

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

Trivial change. Was wondering if having
from stkm.misc import tresult

would make the code look cleaner as opposed to have sktm.misc.tresult everywhere.

Copy link
Contributor

@dzickusrh dzickusrh left a comment

Choose a reason for hiding this comment

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

No code changes, just docs. Looks good

Copy link
Contributor

@dzickusrh dzickusrh left a comment

Choose a reason for hiding this comment

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

Trivial replacement of jobname to self.name. Nice cleanup.

Copy link
Contributor

@dzickusrh dzickusrh left a comment

Choose a reason for hiding this comment

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

another trivial change. Looks good

Copy link
Contributor

@dzickusrh dzickusrh left a comment

Choose a reason for hiding this comment

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

trivial rename. Looks safe.

Copy link
Contributor

@dzickusrh dzickusrh left a comment

Choose a reason for hiding this comment

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

Nice small step change. Again trivial.

Copy link
Contributor

@dzickusrh dzickusrh left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Contributor

@dzickusrh dzickusrh left a comment

Choose a reason for hiding this comment

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

Change is trivial, but changelog implies there was more to it. Why doesn't changelog say, documentation update only?

Copy link
Contributor

@dzickusrh dzickusrh left a comment

Choose a reason for hiding this comment

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

This is really the WIP piece. It doesn't look like it runs. I start to get the idea, but needs examples.

@dzickusrh
Copy link
Contributor

Overall, I personally think the first 7 patches should be merged. It is needed cleanup anyway and might be blocking other changes. The scheduler piece is going to take time to flush out and shouldn't block the first 7 patches.

@spbnick
Copy link
Contributor Author

spbnick commented Jun 6, 2018

Thanks for the review, Don! Yeah, I have the refactoring part in #52, ready for review :)

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.

2 participants