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.

Sign up for GitHub

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

Jump to bottom

Allow parallel requests to take place when non using github.com #1640

Merged
merged 11 commits into from May 5, 2023
Merged

Allow parallel requests to take place when non using github.com #1640

merged 11 commits into from May 5, 2023

Conversation

Roviluca
Copy link
Contributor

@Roviluca Roviluca commented Apr 6, 2023

Resolves #1597

Behavior

Before the change?

  • A locking system was enforcing the client to make api calls one at a time.

After the change?

  • Api call are made in a serial way by default but a new provider setting parallel_requests to let users decide to allow parallel requests
  • Serial requests are enforced in case the api call is made to https://api.github.com

Other information

Additional info

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)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

no

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Feature

Luca Rovinetti added 2 commits April 6, 2023 13:38
rebase_upstream
f74b47b
Add parallel_requests_parameter
3ed3e2c 
The parameter will unlock the possibility to use terraform parallelism but only for Enterprise servers where
rate limiting differ from public Github's one and so the best practices(i.e perform one request per user key per time).
@Roviluca Roviluca changed the title Gh enterprise parallel unlocked Allow parallel requests to take place when non using github.com Apr 6, 2023
kfcampbell
kfcampbell reviewed Apr 10, 2023
View reviewed changes
github/transport.go Show resolved Hide resolved
github/provider.go Show resolved Hide resolved
github/provider.go Show resolved Hide resolved
github/provider.go Outdated Show resolved Hide resolved
github/provider.go Outdated Show resolved Hide resolved
Roviluca and others added 2 commits April 11, 2023 09:44
@Roviluca Roviluca force-pushed the gh_enterprise_parallel_unlocked branch from b173cfe to fb711a8 Compare April 11, 2023 14:56
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 12, 2023
View reviewed changes
github/provider.go Fixed Show resolved Hide resolved
@Roviluca
Copy link
Contributor Author

Fixed the failed check for "Incomplete regular expression for hostnames"

@kfcampbell
Copy link
Member

Can you talk to me more about the smartLock concept? Is the idea that the previously used locking functions are going away and all calls of lock/unlock should switch to the new locking function?

@Roviluca
Copy link
Contributor Author

Yes, that is the idea. I replaced all occurrences of rlt.lock() and rlt.unlock() with a new function smartLock() that accepts a boolean as an input. When True tries to lock, when False unlocks. By default it keeps the same behaviour as before.

The reasons that made me create the new function are:

  • make evident with the name that is not a simple lock but there is some logic inside
  • skip locking/unlocking when rlt.parallelRequests is set to true ( the purpose of the PR )
  • avoided me to add the if rlt.parallelRequests in two places ( the old lock/unlock functions ) or more if I decided to put the if in all invocations

kfcampbell
kfcampbell reviewed Apr 17, 2023
View reviewed changes
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.

Thank you for working with me here. I have two testing and one comment requests for you and then I'll be 👍 on the change: can you please

  • add unit test(s) that interrogate the regex for checking if we're on dotcom
  • add unit test(s) that interrogate the smartLock function
  • make a provider.go change to the description of parallel_requests that makes it clear when you'd want to use it

@Roviluca
Copy link
Contributor Author

Added unit tests for smart_lock and fixed description for parallel_requests.

Regarding add unit test(s) that interrogate the regex for checking if we're on dotcom i have the feeling that it is over testing. The reason is that what I am doing there is just input validation with a very simple regexp.
On the other hand, it may be useful to add an acceptance test for that but, I am having some difficulties wrapping my head around how the acceptance are working. It seems like setting the config var is not changing the behaviour so they seems like not testing anything 🤔.

@kfcampbell
Copy link
Member

kfcampbell commented Apr 20, 2023 •
edited

Added unit tests for smart_lock and fixed description for parallel_requests.

Thank you!

i have the feeling that it is over testing. The reason is that what I am doing there is just input validation with a very simple regexp

That's a reasonable argument, we can leave that test out.

I am having some difficulties wrapping my head around how the acceptance are working. It seems like setting the config var is not changing the behaviour so they seems like not testing anything thinking.

At the moment, acceptance tests are currently run manually and are not triggered on automated PR checks. I am hopeful that this will change in the future.

@Roviluca
Copy link
Contributor Author

Fixed a linting error 👆

@Roviluca
Copy link
Contributor Author

Roviluca commented Apr 26, 2023 •
edited

@kfcampbell is it ok to move forward with this?

kfcampbell
kfcampbell approved these changes May 5, 2023
View reviewed changes
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.

Yep, thanks for working with me here. I'll get this merged and released shortly.

@kfcampbell kfcampbell merged commit 54547e3 into integrations:main May 5, 2023
3 checks passed
@Roviluca Roviluca deleted the gh_enterprise_parallel_unlocked branch June 23, 2023 12:58
avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
@Roviluca @kfcampbell
Allow parallel requests to take place when non using github.com (inte…
fbd18b2 
…grations#1640)

* rebase_upstream

* Add parallel_requests_parameter
The parameter will unlock the possibility to use terraform parallelism but only for Enterprise servers where
rate limiting differ from public Github's one and so the best practices(i.e perform one request per user key per time).

* Update github/provider.go

Co-authored-by: Keegan Campbell <me@kfcampbell.com>

* Add some descriptions and missing error handling

* Fix code quality check's error by adding escapes isGithubDotCom regexp

* Add smart_lock unit tests and fix description for parallel_requests config

* Deferring the unlock in transport test for linter

---------

Co-authored-by: Luca Rovinetti <luca.rovinetti@justeattakeaway.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfcampbell kfcampbell kfcampbell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MAINT]: Enable parallelism to increase performance for self hosted Enterprise servers
2 participants
@Roviluca @kfcampbell