Skip to content

Conversation

@zanella
Copy link

@zanella zanella commented Oct 11, 2022

@gdubicki , sorry for the sloppy code, but I thought it best to ask for feedback 😬

I left TODO comments on things I was not sure about or where you'll probably have a better idea on how to solve the issue I face.

I tested it against a live Gitlab installation, it works, I'll look into writing the tests next.

@zanella zanella had a problem deploying to Integrate Pull Request October 11, 2022 15:18 Failure
Copy link
Member

@gdubicki gdubicki left a comment

Choose a reason for hiding this comment

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

Hey @zanella! Thanks for the PR! I added some comments. We can try jumping on our Slack if you prefer to chat about it. Let me know!

@zanella
Copy link
Author

zanella commented Oct 13, 2022

Hey @zanella! Thanks for the PR! I added some comments. We can try jumping on our Slack if you prefer to chat about it. Let me know!

Yeah, sounds good, do you need to generate an invite link for that ?

@zanella zanella temporarily deployed to Integrate Pull Request October 13, 2022 17:43 Inactive
@gdubicki
Copy link
Member

Hey @zanella! Thanks for the PR! I added some comments. We can try jumping on our Slack if you prefer to chat about it. Let me know!

Yeah, sounds good, do you need to generate an invite link for that ?

https://join.slack.com/t/gitlabform/shared_invite/zt-1i5h4dds0-NO4x63W5_GWL~WWrWy9DQg

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Merging #423 (c3b3b5f) into main (cbb87cd) will increase coverage by 0.00%.
The diff coverage is 93.05%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #423   +/-   ##
=======================================
  Coverage   85.36%   85.37%           
=======================================
  Files          60       62    +2     
  Lines        2228     2291   +63     
=======================================
+ Hits         1902     1956   +54     
- Misses        326      335    +9     
Impacted Files Coverage Δ
gitlabform/gitlab/projects.py 74.61% <ø> (-0.20%) ⬇️
...itlabform/gitlab/project_protected_environments.py 78.57% <78.57%> (ø)
gitlabform/processors/abstract_processor.py 88.70% <95.23%> (+2.99%) ⬆️
gitlabform/transform.py 89.06% <95.45%> (+3.01%) ⬆️
gitlabform/gitlab/__init__.py 100.00% <100.00%> (ø)
gitlabform/gitlab/core.py 94.65% <100.00%> (-1.53%) ⬇️
...tlabform/processors/multiple_entities_processor.py 96.38% <100.00%> (ø)
gitlabform/processors/project/__init__.py 100.00% <100.00%> (ø)
...cessors/shared/protected_environments_processor.py 100.00% <100.00%> (ø)
gitlabform/processors/util/branch_protector.py 76.06% <0.00%> (-1.71%) ⬇️
... and 1 more

@amimas
Copy link
Collaborator

amimas commented Oct 14, 2022

What will the config syntax be for protected environments when this is merged?

@zanella
Copy link
Author

zanella commented Oct 14, 2022

What will the config syntax be for protected environments when this is merged?

"group_name/project_name":
  protected_environments:
    foo:
      name: foo
      deploy_access_levels: &an_anchor_can_be_used_to_reuse #
        - access_level: maintenance # or - number: 40
          #group_inheritance_type: 1
        - user_id: 123  
          #group_inheritance_type: 0
        - user: john_doe
    blah:
       name: blah
       deploy_access_levels: *an_anchor_can_be_used_to_reuse    

@zanella zanella temporarily deployed to Integrate Pull Request October 14, 2022 18:21 Inactive
@zanella zanella had a problem deploying to Integrate Pull Request October 20, 2022 14:20 Failure
@zanella zanella force-pushed the add_protected_environments_processor branch from e8a5d39 to 1f70307 Compare October 20, 2022 15:30
@zanella zanella had a problem deploying to Integrate Pull Request October 20, 2022 15:30 Failure
@zanella zanella force-pushed the add_protected_environments_processor branch from 1f70307 to 50b0ff6 Compare October 20, 2022 15:32
@zanella zanella had a problem deploying to Integrate Pull Request October 20, 2022 15:33 Failure
@zanella zanella force-pushed the add_protected_environments_processor branch from 50b0ff6 to 02b634d Compare October 20, 2022 15:34
@zanella zanella had a problem deploying to Integrate Pull Request October 20, 2022 15:34 Failure
@zanella zanella had a problem deploying to Integrate Pull Request October 20, 2022 15:36 Failure
@zanella zanella had a problem deploying to Integrate Pull Request October 20, 2022 16:21 Failure
@zanella zanella force-pushed the add_protected_environments_processor branch from 60a3238 to 406c936 Compare October 20, 2022 16:29
@zanella zanella had a problem deploying to Integrate Pull Request October 20, 2022 16:29 Failure
@zanella zanella changed the title WIP: Add protected environments processor Add protected environments processor Oct 20, 2022
@zanella zanella temporarily deployed to Integrate Pull Request October 20, 2022 16:59 Inactive
@gdubicki gdubicki force-pushed the add_protected_environments_processor branch from 14721e2 to 4eddd2e Compare October 21, 2022 15:04
@gdubicki gdubicki had a problem deploying to Integrate Pull Request October 21, 2022 15:05 Failure
@gdubicki gdubicki had a problem deploying to Integrate Pull Request October 21, 2022 15:09 Failure
@zanella zanella had a problem deploying to Integrate Pull Request October 21, 2022 15:41 Failure
@gdubicki gdubicki temporarily deployed to Integrate Pull Request October 21, 2022 16:34 Inactive
@zanella zanella had a problem deploying to Integrate Pull Request October 24, 2022 10:10 Failure
@zanella zanella force-pushed the add_protected_environments_processor branch from cc1fe31 to 868ca72 Compare October 27, 2022 18:44
@zanella zanella temporarily deployed to Integrate Pull Request October 27, 2022 18:45 Inactive
@zanella zanella had a problem deploying to Integrate Pull Request October 28, 2022 10:16 Failure
@zanella zanella force-pushed the add_protected_environments_processor branch from 35c834a to 9577d5f Compare October 28, 2022 10:19
@zanella zanella temporarily deployed to Integrate Pull Request October 28, 2022 10:19 Inactive
@gdubicki
Copy link
Member

Final is not compatible with Python 3.7, @zanella...

@zanella zanella force-pushed the add_protected_environments_processor branch from 9577d5f to 808d42d Compare October 28, 2022 12:57
@zanella zanella had a problem deploying to Integrate Pull Request October 28, 2022 12:57 Failure
@zanella zanella force-pushed the add_protected_environments_processor branch from 808d42d to c3b3b5f Compare October 28, 2022 13:17
@zanella zanella temporarily deployed to Integrate Pull Request October 28, 2022 13:17 Inactive
@gdubicki gdubicki had a problem deploying to Integrate Pull Request October 30, 2022 20:59 Failure
@gdubicki
Copy link
Member

I did a fix (there would be a missing link to the new article in the docs' index) and some updates to the docs and now I think we are good to go. Thank you for all the work on this feature, @zanella!

@gdubicki gdubicki merged commit f97025c into gitlabform:main Oct 30, 2022
@gdubicki
Copy link
Member

This code has been pre-released in v3.3.0rc1. Please feel free to check it out before a final release with this that should come within a few days.

@amimas
Copy link
Collaborator

amimas commented Oct 31, 2022

I'm away from work next few weeks. So I can't test it out at the moment but really looking forward to it. Do we want visit the config syntax before this feature is released out of RC?

#423 (comment)

There's a redundant key name, which is treated as a label instead of the actual name of the env. Also the parent config, protected_environments is a departure from protected branch config, which is called branches.

@gdubicki
Copy link
Member

gdubicki commented Oct 31, 2022

There's a redundant key name, which is treated as a label instead of the actual name of the env.

Yes, on second thought it would be nice for this to be cleaned. We could do this relatively easy with a config transformation where:

    protected_branches:
      foo:
        (...)

...would internally be converted to:

    protected_branches:
      foo:
        name: foo
        (...)

Wdyt, @zanella?

Also the parent config, protected_environments is a departure from protected branch config, which is called branches.

I think you are right. It would be better to rename this to just environments, it would be more consistent.

But until we would also implement #164 it might be disappointing to some users that you cannot really manage all the envs but only protect them.

Also it's a bit of a problem with how enforce: true would work - now it unprotects all undefined envs while if this section would be called environments I would expect it to delete all undefined envs...

@zanella zanella deleted the add_protected_environments_processor branch October 31, 2022 12:15
@amimas
Copy link
Collaborator

amimas commented Oct 31, 2022

Also it's a bit of a problem with how enforce: true would work - now it unprotects all undefined envs while if this section would be called environments I would expect it to delete all undefined envs...

Ahh... I see. Didn't know the current implementation detail about this feature. I think you're right. I would also expect enforce: true under environment to delete all the other envs that are not in the config and I can see how that can also be unexpected. We probably need to hash out the details for regular environment config.

But until we would also implement #164 it might be disappointing to some users that you cannot really manage all the envs but only protect them.

This made me think perhaps we should have a separate key under environments to indicate whether an env should be protected or not. For example: protected: true. This also follows the config syntax for branch protection. Protected env is a subset of overall environment feature. So, we can implement the rest of it for #164 later if/as needed.

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.

4 participants