-
Notifications
You must be signed in to change notification settings - Fork 671
Feature/pipeline schedules #398
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
Feature/pipeline schedules #398
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mlq
Thank you for this PR, and sorry for the delay.
Could you have a look at my comments? Thanks!
@@ -1496,6 +1496,18 @@ class ProjectFileManager(BaseManager): | |||
obj_cls = ProjectFile | |||
|
|||
|
|||
class ProjectPipelineSchedule(GitlabObject): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think pipelines schedule are implemented in the v3 API, so this file should probably not be changed.
_obj_cls = ProjectPipelineScheduleVariable | ||
_from_parent_attrs = {'project_id': 'project_id', | ||
'pipeline_schedule_id' : 'id'} | ||
_create_attrs = (('pipeline_schedule_id', 'key', 'value'), tuple()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this line since you redefine _create_attrs on the next line.
_create_attrs = (('pipeline_schedule_id', 'key', 'value'), tuple()) | ||
_create_attrs = (('key', 'value'), tuple()) | ||
|
||
def list(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's a good idea to implement this method. I'd like to avoid adding features that are not directly coming from the GitLab API. I'd rather remove this method.
@@ -2035,6 +2110,7 @@ class Project(SaveMixin, ObjectDeleteMixin, RESTObject): | |||
('notificationsettings', 'ProjectNotificationSettingsManager'), | |||
('pipelines', 'ProjectPipelineManager'), | |||
('protectedbranches', 'ProjectProtectedBranchManager'), | |||
('pipeline_schedules', 'ProjectPipelineSchedulesManager'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use pipelineschedules
for consistency with the rest of the attribute names?
return array | ||
|
||
|
||
class ProjectPipelineSchedule(RESTObject): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitLab supports updating and deleting these resources, so you need to add the SaveMixin
and ObjectDeleteMixin
here.
) | ||
|
||
|
||
class ProjectPipelineSchedulesManager(RetrieveMixin, CreateMixin, RESTManager): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the CRUDMixin
since all the methods are supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the class should be called ProjectPipelineScheduleManager
(no plural).
_from_parent_attrs = {'project_id': 'id'} | ||
_create_attrs = (('description', 'ref', 'cron'), | ||
('cron_timezone', 'active')) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you define the _update_attrs
attributes as well?
_create_attrs = (('description', 'ref', 'cron'), | ||
('cron_timezone', 'active')) | ||
|
||
def create(self, data, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this method (you're just calling the mixin method).
@@ -1706,7 +1706,23 @@ def raw(self, file_path, ref, streamed=False, action=None, chunk_size=1024, | |||
return utils.response_content(result, streamed, action, chunk_size) | |||
|
|||
|
|||
class ProjectPipelineJob(ProjectJob): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pipeline jobs support has already be implemented, could you remove these items?
@mlq if you don't mind I'll update your patch to fix the problems listed in the review. |
Hi! Sorry that I haven't replied yet but I haven't found the time to introduce your suggested changes. If you can do that, it would be great! I just tried to build something that was working for me without knowing everything about the implementation! Thank you! |
Thanks for your fast answer! I'll update your patch. |
Thanks for the initial work @mlq! |
Awesome, thank you for this project! |
Hi,
I've started to implement support for pipeline schedules and wanted to ask if this is wanted to be picked up and reviewed. It allows to add pipeline schedules as well as variables. Any feedback is welcome as I am not so familiar with the code base.
Best regards