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

- bulk update of repository options #939

Merged
merged 11 commits into from
Dec 29, 2020
Merged

Conversation

jgangemi
Copy link
Contributor

@jgangemi jgangemi commented Sep 8, 2020

Description

allows bulk updating of repository settings instead of making individual calls for each.

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

  • Push your changes to a branch other than master. Create your PR from that branch.
  • Add JavaDocs and other comments
  • Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
  • Run mvn clean compile locally. This may reformat your code, commit those changes.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.

When creating a PR:

  • Fill in the "Description" above.
  • Enable "Allow edits from maintainers".

@jgangemi
Copy link
Contributor Author

any updates on getting this merged?

@jgangemi
Copy link
Contributor Author

bueller? as someone who has contributed to this project before, it's super frustrating to do this work and then see the PR languish and other things merged ahead of it.

@jgangemi
Copy link
Contributor Author

@bitwiseman hi! what needs to be done on my end to get some traction on this?

@bitwiseman
Copy link
Member

@jgangemi
Sorry for delay, I was on leave. I've just cut a release, but that is not a comment on this change.

This is a solid, high-quality change. Thanks for including tests and javadoc. Great work!

I've invited you to the hub4j-test-org. Please join and rerecord test files using the org.

Also, as long as you're going to do this level of work, I'd be grateful if you would try implementing this using AbstractBuilder framework that I used in GHLabel. You'll end up with a little smaller set of changes and gain the use o the update()/set() batching behavior from AbstractBuilder. With the inheritance the way you have it now, I think what you have is pretty close already.

private GHCreateRepositoryBuilder getCreateBuilder(String name, boolean useOrg) throws IOException {
GitHub github = getGitHubBeforeAfter();

if (useOrg) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having this be a parameter that gets passed down through the call stack, this should be a setting like mockGitHub.isUseProxy() but on AbstractGitHubWireMockTest, something like isTestWithUserAccount(). This will let users create, debug, and record tests using their account, but also easily turn it off and re-record from the test account later.

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 feel like i followed this from some other example but i can see about addressing this.

@jgangemi
Copy link
Contributor Author

@bitwiseman no worries. i'll take a look at what you suggested and get going on these changes.

@jgangemi jgangemi force-pushed the jae/bulk-update branch 2 times, most recently from 0b153b9 to 89d8df0 Compare December 11, 2020 22:49
@jgangemi
Copy link
Contributor Author

@bitwiseman sorry for the delay on my end, changes made. let me know your feedback.

i set the default behavior to use the "org" to run tests and to set a property for a user account. i updated the contributing doc accordingly.

@bitwiseman bitwiseman merged commit b30d648 into hub4j:master Dec 29, 2020
@jgangemi jgangemi deleted the jae/bulk-update branch December 30, 2020 14:30
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.

2 participants