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

feat : Support repository property for GitHub organization ruleset #2356

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Moser-ss
Copy link
Contributor

@Moser-ss Moser-ss commented Aug 20, 2024

Resolves #2137
Introduces the support to use repository_property to target repositories in the ruleset

The changes were manually tested against a GitHub Organization with an enterprise plan.
It is not possible to add properties with the source system because the lib version doesn't allow that. It is necessary to update to version v65


Before the change?

  • Cannot target ruleset using repository_property

After the change?

  • The ruleset can use the repository_property under the conditions block

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@Moser-ss Moser-ss marked this pull request as draft August 20, 2024 18:57
@Moser-ss Moser-ss marked this pull request as ready for review August 20, 2024 22:58
@Moser-ss Moser-ss force-pushed the Support-repository_property-for-github_organization_ruleset branch from bb62d7a to 40c5111 Compare August 28, 2024 13:17
@Moser-ss Moser-ss changed the title Support repository property for GitHub organization ruleset feat : Support repository property for GitHub organization ruleset Sep 3, 2024
@Moser-ss
Copy link
Contributor Author

Moser-ss commented Sep 7, 2024

@kfcampbell, could you take a quick look to see if I need to improve anything in this PR

Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

Thanks! Have you looked at expanding the tests in github/resource_github_organization_ruleset_test.go to cover this usecase yet?

@Moser-ss
Copy link
Contributor Author

A test case was added, I hope the change was not to ugly

@Moser-ss
Copy link
Contributor Author

Moser-ss commented Oct 5, 2024

@kfcampbell can I do anything to speed up the review of this PR?

Comment on lines +138 to +140
Elem: &schema.Schema{
Type: schema.TypeString,
},
Copy link

@PaarthShah PaarthShah Oct 16, 2024

Choose a reason for hiding this comment

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

Does this handle properties with boolean values? From a json export which shows the use of string "false" and "true" I assume it does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manualy tested the solution and can handle boelean custom_properties using strings

Refactor repository property conditions to make them optional
Flatten update Target parameters to allow the detection of changes when remote resource is updated
Wait for the upgrade to  go-github/v65/github to start use the source property
@stevehipwell
Copy link
Contributor

@Moser-ss I think you need to update the docs to reflect these changes?

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.

[FEAT]: Support targeting dynamic list by properties in organizational rulesets
4 participants