- Notifications
You must be signed in to change notification settings - Fork673
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
| @@ -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 usepipelineschedules 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 theSaveMixin andObjectDeleteMixin 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 theCRUDMixin 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 calledProjectPipelineScheduleManager (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