more specically, I guess all crud operation are inherited from the CRUD mixin but we should ideally implementstop(),protect() andunprotect() methods ?

Codecov Report

Merging#2084 (169375f) intomain (8342f53) willincrease coverage by9.02%.
The diff coverage is92.85%.

@@            Coverage Diff             @@##             main    #2084      +/-   ##==========================================+ Coverage   85.62%   94.64%   +9.02%==========================================  Files          78       78                Lines        5007     5020      +13     ==========================================+ Hits         4287     4751     +464+ Misses        720      269     -451

Flags with carried forward coverage won't be shown.Click here to find out more.

more specically, I guess all crud operation are inherited from the CRUD mixin but we should ideally implementstop(),protect() andunprotect() methods ?

To answer your question:stop() is already implemented on the/environments endpoint, so we're ok - seems like/protected_environments does not have a separate stop endpoint for that.

We would need to add protect/unprotect, yes. If you'd like to add this, you can take a look at the (since deprecated) branch protect methods:9656a16, although that's quite old, you'd probably have a simpler-looking method for that without the custom argument handling.

BTW: There is also a group-level protected environments endpoint:https://docs.gitlab.com/ee/api/group_protected_environments.html

Edit: ah, looking at your PR now, we're actually fine - create/delete from CRUDMixin already cover protecting and unprotecting the environment. But we should maybe document thatcreate()protects anddelete()unprotects the protected environment. This is actually consistent with our protected branches approach to avoid custom code:https://python-gitlab.readthedocs.io/en/stable/gl_objects/protected_branches.html.

I'll also add a note in the docs for our protected branches/tags that create/delete does protect/unprotect.

Thanks for the great PR and quick rework@calve. It'd be cool if you could add a tiny bit of docs for this (see protected branches) if you have the time & energy as well :)

Thanks both of you for the quick review.

BTW: There is also a group-level protected environments endpoint:https://docs.gitlab.com/ee/api/group_protected_environments.html

I'm inclined to let the implementation to a future reader ?

I'm not sure about UpdateMixin - does the API allow PUT on existing protected environments? Even protected branches don't support editing, so I'd think that no.

I'll also add a note in the docs for our protected branches/tags that create/delete does protect/unprotect.

And globally on the create/delete/protect/unprotect behavior, I have to say I did not have those usages right now and so haven't actually tested anything related to other operation than list/get.

I just realised the documentation isn't autogenerated as I initially thought, I'm going to amend the commit to add a specific pages.

And globally on the create/delete/protect/unprotect behavior, I have to say I did not have those usages right now and so haven't actually tested anything related to other operation than list/get.

I just realised the documentation isn't autogenerated as I initially thought, I'm going to amend the commit to add a specific pages.

Thanks for the quick reply@calve. We auto-generate the API reference but add some prose docs with examples usually, as some people find that easier to grok.

Sounds good, the rest can be a follow-up :)

I amended the commit to

• remove the UpdateMixin, and the associated.update() method
• set_create_attrs and_list_filters to what seems to be the actual spec
• document thatcreate() anddelete() actually protect/unprotected existing environments
• manually (and successfully) test a full CRUD cycle on my gitlab-ee instance

please let me know for any other changes or information :)

Thanks! Just a few more typos/nitpicks from me, then we should be good to go. Our handling of IDs is a bit different from how GitLab documents them, because we create objects to manipulate, so I've added some comments.

Updated with your suggestions, thanks for the review.

for reference here are my manual tests

In [12]:p.protected_environments.create({"name":"testenv","deploy_access_levels": [{"access_level":40}]})Out[12]:<ProjectProtectedEnvironmentname:testenv>In [13]:print(p.protected_environments.get("testenv"))<class'gitlab.v4.objects.environments.ProjectProtectedEnvironment'>=> {'name':'testenv','deploy_access_levels': [{'access_level':40,'access_level_description':'Maintainers','user_id':None,'group_id':None,'group_inheritance_type':0}],'required_approval_count':0,'approval_rules': []}In [14]:print(p.protected_environments.list()[0])<class'gitlab.v4.objects.environments.ProjectProtectedEnvironment'>=> {'name':'testenv','deploy_access_levels': [{'access_level':40,'access_level_description':'Maintainers','user_id':None,'group_id':None,'group_inheritance_type':0}],'required_approval_count':0,'approval_rules': []}In [15]:print(p.protected_environments.list()[0].delete())NoneIn [16]:print(p.protected_environments.get("testenv"))

Thanks for the super quick reactions@calve. LGTM now, this should make it into the next scheduled release on the 28th.

Thanks for your time, the super review and velocity 🤝