Skip to content
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

#454 Keep bot members in place regardless of "enforce" setting #544

Merged
merged 3 commits into from
May 19, 2023

Conversation

lhokktyn
Copy link
Contributor

This PR aims to address the issue described in #454. I've gone for a keep_bots option which seems more descriptive of what it actual does, rather than the ignore_bot_members mentioned in the original issue. Happy to change this is if needs be.

Python isn't my first language so I've largely picked through what exists already and taken a steer from that. Any guidance on improvements would be much appreciated :)

@lhokktyn lhokktyn temporarily deployed to Integrate Pull Request May 12, 2023 10:57 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #544 (846de95) into main (b2161f8) will increase coverage by 40.05%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #544       +/-   ##
===========================================
+ Coverage   42.03%   82.09%   +40.05%     
===========================================
  Files          70       70               
  Lines        2650     2658        +8     
===========================================
+ Hits         1114     2182     +1068     
+ Misses       1536      476     -1060     
Impacted Files Coverage Δ
...abform/processors/group/group_members_processor.py 98.88% <100.00%> (+83.77%) ⬆️
gitlabform/processors/project/members_processor.py 89.85% <100.00%> (+72.93%) ⬆️

... and 57 files with indirect coverage changes

@gdubicki
Copy link
Member

Hey @lhokktyn, this PR looks excellent! The code is clean and stylish, you updated the docs and added the tests, which are passing of course - everything is in place! 🥳 Let me merge this now and I will make a release with this feature later over the weekend.

Python isn't my first language so I've largely picked through what exists already and taken a steer from that. Any guidance on improvements would be much appreciated :)

Oh, man, you are too modest! I really don't have any suggestions for you, you did great. But perhaps you can share your experience with contributing to the project? Were the contribution docs helpful, if you read them? Was the code easy to extend? Anything that we could improve?

@gdubicki
Copy link
Member

gdubicki commented May 13, 2023

I just noticed that this PR is technically still a draft... Do you want to add anything more, @lhokktyn, or can I switch it to "ready to merge" and merge?

@lhokktyn
Copy link
Contributor Author

Oh, man, you are too modest! I really don't have any suggestions for you, you did great. But perhaps you can share your experience with contributing to the project? Were the contribution docs helpful, if you read them? Was the code easy to extend? Anything that we could improve?

Thanks @gdubicki, that's much appreciated :)

It was honestly very straightforward to get a development environment up and running - the docs just worked as written! The suites of tests is comprehensive (as are the CI checks) which provides a great sense of safety when it comes to making changes. I found the code well modularised, and easy to navigate to find the parts I needed. Most of my time was actually spent familiarising myself with some of pytest's features like fixture scoping and yielding, and the -s flag came in very handy to get some basic console output spitting out (I didn't venture into using a Python debugger).

I just noticed that this PR is technically still a draft... Do you want to add anything more, @lhokktyn, or can I switch it to "ready to merge" and merge?

Some colleagues are doing a bit of testing internally just to verify this change actually addresses the issue we were facing, but once that's done (hopefully this week) then I'll come and flag as ready. Hope that's ok.

@lhokktyn
Copy link
Contributor Author

@gdubicki we've tested in house and good to go!

@lhokktyn lhokktyn marked this pull request as ready for review May 17, 2023 16:37
@gdubicki gdubicki merged commit b2d6214 into gitlabform:main May 19, 2023
25 checks passed
@gdubicki
Copy link
Member

Released in v3.6.0.

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.

None yet

2 participants