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

Add github_branch_protection #301

Merged
merged 2 commits into from
Mar 2, 2021
Merged

Conversation

eliecharra
Copy link
Contributor

Q A
πŸ› Bug fix? no
πŸš€ New feature? yes
⚠ Deprecations? no
❌ BC Break no
πŸ”— Related issues #249
❓ Documentation yes

Description

⚠️ To be able to alert when use user github_branch_protection_v3, we should create a resource, a deserializer and a finally a middleware to check if there is any github_branch_protection_v3 resource in state just to send an alert and remove theses resources. Do we really want to do that ? Maybe a line in LIMITATIONS.md may be enough.

⚠️ It seems that the terraform provider is not returning the repository_id, so I've ignored it from the diff

Attentive review required please @moadibfr @wbeuil

@eliecharra eliecharra added the kind/enhancement New feature or improvement label Feb 26, 2021
@eliecharra eliecharra requested a review from a team February 26, 2021 11:21
@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #301 (1c34d86) into main (fcc17eb) will increase coverage by 0.11%.
The diff coverage is 71.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
+ Coverage   68.92%   69.03%   +0.11%     
==========================================
  Files         269      273       +4     
  Lines        5724     5796      +72     
==========================================
+ Hits         3945     4001      +56     
- Misses       1436     1447      +11     
- Partials      343      348       +5     
Impacted Files Coverage Ξ”
pkg/remote/github/init.go 0.00% <0.00%> (ΓΈ)
pkg/remote/github/provider.go 7.69% <40.00%> (+7.69%) ⬆️
...remote/github/github_branch_protection_supplier.go 42.85% <42.85%> (ΓΈ)
...erializer/github_branch_protection_deserializer.go 69.23% <69.23%> (ΓΈ)
...kg/resource/github/github_branch_protection_ext.go 87.50% <87.50%> (ΓΈ)
pkg/iac/deserializers.go 100.00% <100.00%> (ΓΈ)
pkg/remote/github/repository.go 90.82% <100.00%> (+1.93%) ⬆️
pkg/resource/github/github_branch_protection.go 100.00% <100.00%> (ΓΈ)
... and 2 more

Copy link
Contributor

@lotoussa lotoussa left a comment

Choose a reason for hiding this comment

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

~LGTM

if repoId == "" {
return fmt.Sprintf("Branch: %s (Id: %s)", *r.Pattern, r.Id)
}
return fmt.Sprintf("Branch: %s, RepoId: %s", *r.Pattern, repoId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't missing parenthesis for RepoId ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question, WDYT @moadibfr @wbeuil ? Should we display Branch: %s (RepoId: %s) to be more consistent with Branch: %s (Id: %s) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be consistent then. But I'm fine with one or the other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@moadibfr
Copy link
Contributor

warning To be able to alert when use user github_branch_protection_v3, we should create a resource, a deserializer and a finally a middleware to check if there is any github_branch_protection_v3 resource in state just to send an alert and remove theses resources. Do we really want to do that ? Maybe a line in LIMITATIONS.md may be enough.

Maybe we could add a warning when we find unmanaged repository only ? Saying that this might be a false positive as we don't support v3 ?

doc/LIMITATIONS.md Outdated Show resolved Hide resolved
doc/LIMITATIONS.md Outdated Show resolved Hide resolved
doc/LIMITATIONS.md Outdated Show resolved Hide resolved
doc/LIMITATIONS.md Outdated Show resolved Hide resolved
doc/LIMITATIONS.md Outdated Show resolved Hide resolved
@eliecharra
Copy link
Contributor Author

Maybe we could add a warning when we find unmanaged repository only ? Saying that this might be a false positive as we don't support v3 ?

We cannot make the difference between a false positive drift and a real drift with this approach.
For me we the only way to handle this is to read v3 res from the state and try to match them to non-v3 unmanaged one and ignore them with a warning. So we'll end up with a v3 resource and deserializer without any supplier related.

WDYT @moadibfr @wbeuil ? Is it worth it ? I can totally do this in a couple of hours.

@wbeuil
Copy link
Contributor

wbeuil commented Mar 2, 2021

I'm not sure it's worth your time to do it. Are we sure that we can create a non-v3 resource with a v3 resource ? Can't we just warn the user if we find a v3 resource in its state ?

wbeuil
wbeuil previously approved these changes Mar 2, 2021
moadibfr
moadibfr previously approved these changes Mar 2, 2021
@eliecharra eliecharra dismissed stale reviews from moadibfr and wbeuil via 468a5e6 March 2, 2021 13:08
@eliecharra eliecharra force-pushed the add_github_branch_protection branch 2 times, most recently from 468a5e6 to ca6c9cc Compare March 2, 2021 13:36
@eliecharra eliecharra force-pushed the add_github_branch_protection branch from 7f5cbdf to 7d0a28c Compare March 2, 2021 14:45
@eliecharra eliecharra merged commit 05b53ae into main Mar 2, 2021
@eliecharra eliecharra deleted the add_github_branch_protection branch March 2, 2021 14:52
@eliecharra eliecharra linked an issue Mar 2, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for github_branch_protection
4 participants