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!: Add support for enterprise rulesets #3417

Merged
merged 12 commits into from
Jan 9, 2025

Conversation

stevehipwell
Copy link
Contributor

@stevehipwell stevehipwell commented Jan 6, 2025

BREAKING CHANGE: Create*Ruleset and Update*Ruleset now pass ruleset parameter by-value instead of by-reference.

Fixes: #3416.

@gmlewis gmlewis changed the title feat: Added enterprise rules feat: Add support for enterprise rulesets Jan 6, 2025
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.26%. Comparing base (2b8c7fa) to head (0e2ee69).
Report is 219 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3417      +/-   ##
==========================================
- Coverage   97.72%   92.26%   -5.46%     
==========================================
  Files         153      174      +21     
  Lines       13390    15023    +1633     
==========================================
+ Hits        13085    13861     +776     
- Misses        215     1068     +853     
- Partials       90       94       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @stevehipwell .
In addition to the linter findings, please make the following changes:

github/enterprise_rules.go Outdated Show resolved Hide resolved
github/orgs_rules_test.go Outdated Show resolved Hide resolved
github/orgs_rules_test.go Outdated Show resolved Hide resolved
Signed-off-by: Steve Hipwell <[email protected]>
@gmlewis
Copy link
Collaborator

gmlewis commented Jan 6, 2025

Please don't use force-push to PRs in this repo as explained in CONTRIBUTING.md, as it makes it difficult for reviewers to see what has changed since the last review. We always use squash-and-merge in this repo, so commit history will always be clean. Thank you for your consideration and understanding.

@stevehipwell
Copy link
Contributor Author

@gmlewis sorry for the force pushing, it's muscle memory.

@stevehipwell
Copy link
Contributor Author

@gmlewis based on the last run it looks like the OpenAPI schema needs to be updated?

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 6, 2025

@gmlewis based on the last run it looks like the OpenAPI schema needs to be updated?

I'll see if I can update them locally and report back my findings.

@gmlewis gmlewis mentioned this pull request Jan 6, 2025
@gmlewis
Copy link
Collaborator

gmlewis commented Jan 6, 2025

@gmlewis based on the last run it looks like the OpenAPI schema needs to be updated?

I'll see if I can update them locally and report back my findings.

@stevehipwell - I just created #3419. Could you please see if this looks right to you?

@stevehipwell
Copy link
Contributor Author

@gmlewis how would you like me to update this PR so the metadata is correct? Usually I'd rebase the commit onto the repo changes.

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 6, 2025

@gmlewis how would you like me to update this PR so the metadata is correct? Usually I'd rebase the commit onto the repo changes.

If you git pull the master branch, then you can git merge master in this branch and simply push to the PR.

@stevehipwell
Copy link
Contributor Author

If you git pull the master branch, then you can git merge master in this branch and simply push to the PR.

Done, but will GH give you a linear history on squash?

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 7, 2025

Done, but will GH give you a linear history on squash?

Squash gives us a very clean linear history of PRs. Check out this repo's git log to see the result.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jan 7, 2025
github/repos_rules.go Outdated Show resolved Hide resolved
github/repos_rules.go Outdated Show resolved Hide resolved
@gmlewis gmlewis changed the title feat: Add support for enterprise rulesets feat!: Add support for enterprise rulesets Jan 7, 2025
@gmlewis gmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Jan 7, 2025
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

One more nit... would you mind renaming the parameter rs Ruleset to ruleset Ruleset in this PR to make the docs a bit more readable, please?

Oh, and with that last comment I sent - if you still want to change the signature to the deprecated endpoint for consistency, that is fine with me... so maybe just update the comment and ignore the rest.

github/repos_rules.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @stevehipwell !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

Copy link
Contributor

@dnwe dnwe left a comment

Choose a reason for hiding this comment

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

feature implementation looks good to me and matches the docs

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jan 9, 2025
@gmlewis
Copy link
Collaborator

gmlewis commented Jan 9, 2025

Thank you, @dnwe !
Merging.

@gmlewis gmlewis merged commit a61e9cf into google:master Jan 9, 2025
6 of 7 checks passed
@stevehipwell stevehipwell deleted the enterprise-rules branch January 9, 2025 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add enterprise rules support
3 participants