Skip to content

introduce RefreshMixin #426

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 1 commit into from
Mar 5, 2018
Merged

introduce RefreshMixin #426

merged 1 commit into from
Mar 5, 2018

Conversation

tardyp
Copy link
Contributor

@tardyp tardyp commented Feb 5, 2018

RefreshMixin allows to update a REST object so that you can poll on it.
This is mostly useful for pipelines and jobs, but could be set on most of other objects, with unknown usecases.

@@ -333,6 +333,22 @@
pipeline = project.pipelines.create({'ref': 'master'})
# end pipeline create

# pipeline trigger
def get_or_create_trigger(project):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this could be a nice improvement to add this get_or_create to trigger manager


trigger = get_or_create_trigger(project)
pipeline = project.trigger_pipeline('master', trigger.token, variables={"DEPLOY_ZONE": "us-west1"})
while pipeline.finished_at is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, we could add a helper like pipeline.wait_for_finish(poll_freq=1)

@tardyp
Copy link
Contributor Author

tardyp commented Feb 9, 2018

@gpocentek could you please give some feedback?

@gpocentek
Copy link
Contributor

Hi @tardyp

Sorry for my late answer.

If I understand correctly the goal of the ReadMixin is to refresh the attributes an existing object. I like the idea but I wonder if adding a mixin is really the best option. What do you think of adding a read (or refresh?) method the the existing GetMixin? This way all the objects that you can GET could have the possibility of being refreshed.

Does that sound good to you?

Thanks for your work :)

@tardyp
Copy link
Contributor Author

tardyp commented Mar 3, 2018

@gpocentek I like the refresh name method better than read. Thanks for the suggestion.

I think that the ReadMixin is only for managers. Here we want to add this mixin to restobjects.
We can add this method directly to RESTObject, without need for a new mixin.

Let me know your though, I'll update the PR after we get an agreement.

@gpocentek
Copy link
Contributor

@tardyp you're right, using GetMixin doesn't make sense.

I don't think it will be possible to refresh all the objects, so I guess providing a new RefreshMixin with a refresh method is the way to go.

Thank you for the update.

@tardyp tardyp changed the title introduce ReadMixin introduce RefreshMixin Mar 4, 2018
RefreshMixin allows to update a REST object so that you can poll on it.
This is mostly useful for pipelines and jobs, but could be set on most of other objects, with unknown usecases.
@tardyp
Copy link
Contributor Author

tardyp commented Mar 4, 2018

Hi @gpocentek I updated my code. I believe the e2e tests are still a little bit flaky.

@gpocentek
Copy link
Contributor

Hi @tardyp

The tests have been a problem for a little while now, but I didn't have time to try to understand why yet.

Do you mind removing the change on the test script? I don't think it will really help, and I'd prefer to have this kind of change in a separate path.

Apart from that the patch looks good, thanks!

@tardyp
Copy link
Contributor Author

tardyp commented Mar 5, 2018

@gpocentek done

@gpocentek
Copy link
Contributor

Thanks!

@gpocentek gpocentek merged commit ee4591d into python-gitlab:master Mar 5, 2018
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