Skip to content

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

Merged
merged 2 commits into from
Feb 5, 2018

Conversation

mlq
Copy link
Contributor

@mlq mlq commented Dec 13, 2017

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

Copy link
Contributor

@gpocentek gpocentek left a 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):
Copy link
Contributor

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())
Copy link
Contributor

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):
Copy link
Contributor

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'),
Copy link
Contributor

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):
Copy link
Contributor

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):
Copy link
Contributor

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.

Copy link
Contributor

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'))

Copy link
Contributor

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):
Copy link
Contributor

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):
Copy link
Contributor

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?

@gpocentek
Copy link
Contributor

@mlq if you don't mind I'll update your patch to fix the problems listed in the review.

@mlq
Copy link
Contributor Author

mlq commented Feb 5, 2018

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!

@gpocentek
Copy link
Contributor

Thanks for your fast answer! I'll update your patch.

@gpocentek gpocentek merged commit b861837 into python-gitlab:master Feb 5, 2018
@gpocentek
Copy link
Contributor

Thanks for the initial work @mlq!

@mlq
Copy link
Contributor Author

mlq commented Feb 5, 2018

Awesome, thank you for this project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants