Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Change github objects in set to only strings #67

Merged
merged 2 commits into from
Jan 26, 2021

Conversation

xtuchyna
Copy link
Member

Related Issues and Dependencies

Quick fix for recent PR #58 (PyGithub objects do not support hashing -> mentioned it here PyGithub/PyGithub#1826 ).

@sesheta sesheta requested a review from KPostOffice January 21, 2021 23:57
@sesheta sesheta added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 21, 2021
Copy link

@pacospace pacospace left a comment

Choose a reason for hiding this comment

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

lgtm, one question, do we have env variables to allow mi-scheduler to schedule only some specific tasks?

for example having env variables that can deactivate/activate (0 not active, 1 active) one task if we don't want to schedule everything at the same time, or if we analyze these things in different clusters and don't want to perform double work?

SCHEDULE_GH_REPO_ANALYSIS (in the future we can extend it to other SCHEDULE_GL_REPO_ANALYSIS for GitLab)

SCHEDULE_KEBECHET_ANALYSIS

wdyt?

Copy link
Member

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

One small comment

@@ -48,9 +47,10 @@ class Schedule:
def __init__(self, github: Github, organizations: List[str] = None, repositories: List[str] = None):
Copy link
Member

Choose a reason for hiding this comment

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

Just set the default for organizations and repositores to be [] here

Copy link
Member Author

Choose a reason for hiding this comment

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

python is known to work unexpectedly here: if you use mutable as a default parameter, it can be modified later, which would affect all the other subsequent calls.

See here e.g.: https://web.archive.org/web/20200221224620/http://effbot.org/zone/default-values.htm

Therefore, for a better practice, I use None and then check, instead of mutables as default (even though they are not modified in body).

@xtuchyna
Copy link
Member Author

xtuchyna commented Jan 25, 2021

@pacospace hmm, not sure if I understand correctly

for example having env variables that can deactivate/activate (0 not active, 1 active) one task if we don't want to schedule everything at the same time

Doesn't that do the limit setting in the openshift template (max. number of workflows allowed)?
Or do you mean something like a semaphore (that let's say one repository has a precedence over another) ?

if we analyze these things in different clusters and don't want to perform double work?

if there are two mi-scheduler cronjobs in two different clusters, setting up the env variables to avoid duplicit workflows would not work, or am i mistaken?

@pacospace
Copy link

pacospace commented Jan 25, 2021

@pacospace hmm, not sure if I understand correctly

for example having env variables that can deactivate/activate (0 not active, 1 active) one task if we don't want to schedule everything at the same time

Doesn't that do the limit setting in the openshift template (max. number of workflows allowed)?
Or do you mean something like a semaphore (that let's say one repository has a precedence over another) ?

if we analyze these things in different clusters and don't want to perform double work?

if there are two mi-scheduler cronjobs in two different clusters, setting up the env variables to avoid duplicit workflows would not work, or am i mistaken?

My comment is related to schedule specific analysis like a semaphore. Imagine for example:

cluster 1: SCHEDULE_GH_REPO_ANALYSIS=1, SCHEDULE_KEBECHET_ANALYSIS=0

cluster 2: SCHEDULE_GH_REPO_ANALYSIS=0, SCHEDULE_KEBECHET_ANALYSIS=1

See for example what we do for graph-refresh-job: https://github.com/thoth-station/graph-refresh-job/blob/d090fada6ba13ee212efc5729bf51bd3c45ba532/producer.py#L63

@xtuchyna
Copy link
Member Author

@pacospace yes, that's a good idea :) Issue created

@xtuchyna
Copy link
Member Author

/approve

@sesheta
Copy link
Member

sesheta commented Jan 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xtuchyna

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2021
@sesheta sesheta merged commit 9cf4de6 into thoth-station:master Jan 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants