-
Notifications
You must be signed in to change notification settings - Fork 736
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 repository and organization rulesets #1808
Conversation
@o-sama I'm game to collaborate, but also not sure how to do that most effectively 😅 I guess a good place to start would be to read each others PR:s and see which parts looks the best. And also agreeing on how the Terraform Schema should look. Since you already have the org level stuff up and running, I think it's probably easier to base whatever changes we make on your PR. I'll take a look at your code tomorrow after work (around 18 CET ish) |
@felixlut sounds good, feel free to make changes to the branch, I’ll add you as a collaborator. I’ll also add an example resource here in the morning so you can check it out. I think your use of a utils file for the ruleset functions is smart, we can definitely extract most of the common logic out of the two resource helpers into that. If we end up needing to chat/call we can set up something to help us collaborate better 😄 |
@felixlut Feel free to add me on discord, username is |
Made a change to use shared utils between repo and org rulesets, with a bool used to check for things specific to each, thanks for the idea @felixlut, picking the best from both of our changes 😄 Here’s an example ruleset which works on my end:
We should probably:
|
@o-sama I had no idea you could just create multiple blocks of the same field like you did below. IMO we should expose required_check {
context = "ci"
}
required_check {
context = ""
} I think conditions {
include = ["~ALL"]
exclude = []
} I think the data_sources can be a separate PR.
|
Description: "Parameters for a repository ruleset ref name condition.", | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"ref_name": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I mentioned in #1808 (comment), I think we can drop the ref_name
. It's just cluttering the usability imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one might be useful to keep in case it’s an org ruleset, those have include
and exclude
if you use repository_name
, so it could be more clear to have ref_name
there so the inner parameters aren’t at the top level and also in repository_name
, but I have no strong preference here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. Now that I've looked at the org level one a bit, I think it's fine to keep the ref_name
there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, we can keep this in mind and then check with others once we’re ready to have this reviewed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me
Optional: true, | ||
Description: "Prevent users with push access from force pushing to branches.", | ||
}, | ||
"commit_message_pattern": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is a feature not available trough the UI yet? I couldn't see it when I tested my solution out atleast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is available on org rulesets as well as repo rulesets for repos owned by an org, we should validate somewhere but it’d have to be outside of the schema’s validateFunc
I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a sanity check function, lets say validateRulesetResource()
, which is called as the first step of all the CRUD operations. Something like I did here
func validatePortfolioResource(d *schema.ResourceData) error {
switch selectionMode := d.Get("selection_mode"); selectionMode {
case "NONE", "MANUAL", "REST":
return nil
case "TAGS":
tags := d.Get("tags").([]interface{})
if len(tags) == 0 {
return fmt.Errorf("validatePortfolioResource: When selection_mode is set to TAGS, you need atleast 1 tag, got: %+v", d.Get("tags"))
}
for _, tag := range d.Get("tags").([]interface{}) {
tagString := fmt.Sprint(tag)
if len(tagString) == 0 {
return fmt.Errorf("validatePortfolioResource: When selection_mode is set to TAGS, each tag must be non 0, got: %s", tagString)
}
}
return nil
case "REGEXP":
regexp := d.Get("regexp").(string)
if len(regexp) == 0 {
return fmt.Errorf("validatePortfolioResource: When selection_mode is set to REGEXP, regexp must be set, got: \"%s\"", regexp)
}
return nil
default:
return fmt.Errorf("resourceSonarqubePortfolioCreate: selection_mode needs to be set to one of NONE, MANUAL, TAGS, REGEXP, REST")
}
}
func resourceSonarqubePortfolioCreate(d *schema.ResourceData, m interface{}) error {
if err := checkPortfolioSupport(m.(*ProviderConfiguration)); err != nil {
return err
}
// ...
}
return nil | ||
} | ||
if ghErr.Response.StatusCode == http.StatusNotFound { | ||
log.Printf("[INFO] Removing ruleset %s/%s: %d from state because it no longer exists in GitHub", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a big nono in Terraform Providers. Automatically removing stuff from the state feels wrong. I think it should just crash, but I'm not sure what the best practices are here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I took out of resource_repository
, it’s essentially used for state refresh, if we can’t find it then it’s not there anymore and we update the state accordingly, at least that’s what I gathered from the current implementation of the provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I might be wrong on that point
Required: true, | ||
Description: "The ID of the actor that can bypass a ruleset", | ||
}, | ||
"actor_type": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ValidateFunc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in o-sama#1
Feat repo rules quickfixes
@kfcampbell How would I go about automated testing for enterprise features? I’m looking specifically at org-level rulesets and repo rulesets within an enterprise (metadata rules don’t work on regular orgs). I’ve tested manually and confirmed the functionality works. |
@kfcampbell Sounds good, I’ll write something like that. @felixlut I closed google/go-github#2836 since I got a response regarding the update rule, it should have the existing parameter but that only works for forked repos (but currently bugged anyways). So, I’ll add the parameter and retest that non-forked repos work, and it should be testable for forked repos once it’s fixed on the GitHub side. I wanted to push a new commit today but I’ve not been feeling well, so I’ll just add this stuff onto my next commit and do that for tomorrow/Wed. |
@kfcampbell I’ve added the enterprise tests here:
Let me know if I need to fix anything or if threre’s something else needed on my end 🙂 @felixlut I’ve changed the import test to cover a ruleset with several rules enabled and added a field for the forked-repo-only param for update rule. I have a PR open in the |
9f1ec87
to
faf2522
Compare
faf2522
to
0283832
Compare
@kfcampbell @felixlut This should be good for review now. Updated the provider to use |
if ghErr.Response.StatusCode == http.StatusNotModified { | ||
return nil | ||
} | ||
if ghErr.Response.StatusCode == http.StatusNotFound { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read through the whole thing again now, and it lgtm.
@kfcampbell This is now ready to be merged at your convenience provided the acceptance tests are successful when you run them, especially the enterprise ones since I can’t guarantee they’re good myself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you both for all the effort here! The collaboration is wonderful to see. I'll get this merged now and released sometime next week!
Hi, thank you for implementing this. |
Super timely addition for me! Thank you. "inlcude" instead of "include" What's the compatibility-promise around fixing this? Do we need to now support both properties with one deprecated? Is it something you can correct in a patch release and just assume the usage is low? |
@csainty Since the release was cut only a few days ago, I think it's better that we cut a patch release to fix it, better to support only the correct spelling in the long run. Also, apologies for that! Had to work on this a bunch during the nights between work and baby duties. I just opened #1884 to fix this and I'm open to supporting both versions if that's the direction we want to go about it. |
Awesome, thanks. And no need to apologise, a very easy typo to miss. I only spotted it when my config complained there was no Some other feedback from my first tests with this.
The config I am aiming to write looks like this import {
to = github_organization_ruleset.my_ruleset
id = "123456"
}
resource "github_organization_ruleset" "my_ruleset" {
name = "My example ruleset"
target = "branch"
enforcement = "active"
bypass_actors {
actor_type = "Integration"
actor_id = "123456"
}
conditions {
repository_id = [
123,
456,
789,
]
ref_name {
include = ["~DEFAULT_BRANCH"]
exclude = []
}
}
rules {
deletion = true
non_fast_forward = true
pull_request {
required_approving_review_count = 1
dismiss_stale_reviews_on_push = true
require_code_owner_review = true
}
}
} Basically I have a ruleset I selectively apply to some repositories where I want to give a GitHub App installation permission to bypass the rules. When I plan (with the MaxItems edit inplace) I get # github_organization_ruleset.my_ruleset will be updated in-place
# (imported from "123456")
~ resource "github_organization_ruleset" "my_ruleset" {
enforcement = "active"
etag = "<snip>"
id = "123456"
name = "My example ruleset"
node_id = "<snip>"
ruleset_id = 123456
target = "branch"
+ bypass_actors {
+ actor_id = 123456
+ actor_type = "Integration"
}
conditions {
repository_id = [
123,
456,
789,
]
ref_name {
exclude = []
include = [
"~DEFAULT_BRANCH",
]
}
}
rules {
creation = false
deletion = true
non_fast_forward = true
required_linear_history = false
required_signatures = false
update = false
pull_request {
dismiss_stale_reviews_on_push = true
require_code_owner_review = true
require_last_push_approval = false
required_approving_review_count = 1
required_review_thread_resolution = false
}
}
} Note the |
@csainty thanks for the feedback man, much appreciated! There's only so much manual testing one can do and I don't have a test enterprise to run automated tests on. I'll look into the stuff you raised and get to working on it tonight! |
* Add repository and organization rulesets (WIP) * Use shared utils between repo and org rulesets * Fix required status checks * remove unneeded print * add owner field * add validation to actor_type * add validation to target and cleanup organziation schema a bit * implement org level rulesets * remove repository rules mentioning of 'enabled' * remove org level 'enabled' * Remove explicit owner for rulesets, minor fixes * State imports and repo rulesets tests * Add docs, update tests, small changes to rulesets * Update rulesets to go-github v54 --------- Co-authored-by: felixlut <felix.luthman@gmail.com> Co-authored-by: Keegan Campbell <me@kfcampbell.com>
Resolves #1777
Before the change?
Please see our docs on breaking changes to help!
This is also being worked on in #1807, seems we started working on it around the same time but came to different implementations and we can pair on this if preferred, @felixlut. Not 100% sure what the best way to handle this is 🙂