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

WIP: initial work on CD #3

Open
wants to merge 12 commits into
base: github_apps
from

Conversation

Projects
None yet
2 participants
@TrueBrain
Owner

TrueBrain commented Jul 11, 2018

No description provided.

TrueBrain added some commits Jul 10, 2018

Add documentation why this CD exists, and how it is implemented
This to be more clear why a new CD was created, and why not an existing was used
@LordAro

first pass at a review

try:
await github.process_request(headers, data)
return web.Response(status=http.HTTPStatus.OK)
except Exception:

This comment has been minimized.

@LordAro

LordAro Jul 12, 2018

catching Exceptions is generally considered Bad Form, because it catches everything, including keyboard interrupts

This comment has been minimized.

@TrueBrain

TrueBrain Jul 12, 2018

Owner

Generally, yes. In this case, there is nothing else. I want that on any error, the exception is logged and the user is served an internal server error. Funny enough, if I wouldn't do it here, aiohttp would do it himself a bit later. As you don't want your application to crash if 1 request fails to be processed. So there is not much else.

This comment has been minimized.

@TrueBrain

TrueBrain Jul 17, 2018

Owner

Also, I found out, that KeyboardInterrupt is handled by asyncio, and always work. This because these entries are in coroutines, and not in the main thread/routine :D

async def runner_send_event(self, event, data=None):
payload = {
"type": event,

This comment has been minimized.

@LordAro

LordAro Jul 12, 2018

is this expected to get a lot longer? if not, might as well make it one line

"""Order DorpsGek to execute certain commands."""
if data not in _registry.keys():
raise YAMLConfigurationError(f"'dorpsgek' has value '{data}'; supported: %s" % ",".join(_registry.keys()))

This comment has been minimized.

@LordAro

LordAro Jul 12, 2018

oldstyle string formatting

This comment has been minimized.

@TrueBrain

TrueBrain Jul 12, 2018

Owner

Without adding an additional variable, not much to do about it. Using f-string in 90% of the cases doesn't mean the %-style is bad from time to time :) Especially in error-flows :)

try:
func(job, data)
except YAMLConfigurationError as err:
errors.append("%s in job '{job_name}'" % err.args)

This comment has been minimized.

@LordAro

LordAro Jul 12, 2018

oldstyle string formatting, and it's not an f-string :)

@LordAro

LGTM, I'll leave actual functional testing to you :)

General thoughts:
Bit of code duplication between runner & github - notably load_config
(Unit) tests? Not sure how feasible they are

logging.basicConfig(level=logging.INFO)
load_config()
for module in config.MODULES.split() + MODULES_ALWAYS_LOAD:

This comment has been minimized.

@LordAro

LordAro Jul 20, 2018

Have you thought about module load order at all? I'd expect the "always load" modules to be loaded before any config modules

RUNNER_PORT = 8081
MODULES = ""
DORPSGEK_COMMANDS = ""

This comment has been minimized.

@LordAro

LordAro Jul 20, 2018

the format of these isn't mentioned anywhere - it calls split() on them, but that's not clear from just the config file..

def load_config():
for key in dir(config):
# Only accept config entries that start with a capital
if key[0] < "A" or key[0] > "Z":

This comment has been minimized.

@LordAro

LordAro Jul 20, 2018

not ("A" < key[0] < "Z") ?
Also, silently ignoring other config entries?

for key in dir(config):
# Only accept config entries that start with a capital
if key[0] < "A" or key[0] > "Z":

This comment has been minimized.

@LordAro

LordAro Jul 20, 2018

same as above

async def schedule_task(jobs_to_execute, repository_name, ref, clone_url):
task_coroutine = execute_task(jobs_to_execute, repository_name, ref, clone_url)
log.info("Scheduling new task; %d in queue now", task_list.qsize() + 1)

This comment has been minimized.

@LordAro

LordAro Jul 20, 2018

+1 looks a bit odd, maybe log after the put_nowait?

from dorpsgek_github.core.yaml import registry as yaml
from dorpsgek_github.core.yaml.exceptions import YAMLConfigurationError
OPTIONS = ["manual"]

This comment has been minimized.

@LordAro

LordAro Jul 20, 2018

SUPPORTED_OPTIONS?

"""
When this job should execute.
Currnetly only 'manual' is valid, meaning the job needs a manual trigger before running.

This comment has been minimized.

@LordAro
return_code = await process.wait()
if return_code != 0:
stderr = []
while True:

This comment has been minimized.

@LordAro

LordAro Jul 20, 2018

Why is this done in an (await) loop? The process has already finished, has it not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment