-
Notifications
You must be signed in to change notification settings - Fork 922
[BUG] Fix eventual consistency of repository create API #3060
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
base: main
Are you sure you want to change the base?
[BUG] Fix eventual consistency of repository create API #3060
Conversation
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Resolves integrations#2604 Signed-off-by: Timo Sand <timo.sand@f-secure.com>
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
9e4b14c to
379dc91
Compare
stevehipwell
left a comment
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.
@deiga is this specifically to fix the template create issues? If so then I could get behind waiting for the repo for that specific case, but I 100% wouldn't want additional API calls when creating a new repo from scratch.
The initial fix for issues like this should be to remove the call to read after create/update pattern and to separate resources out that use multiple different API endpoints.
|
@stevehipwell This is to fix ANY interaction with the repository after it's been created. We have a bunch of bug reports about flakyness around repo creation and I assume it's because of https://github.com/google/go-github/blob/v81.0.0/github/repos.go#L528-L531 We could of course add a boolean field to allow users to toggle the wait if you think that makes sense |
|
@deiga we should do all of the other fixes before even considering increasing the API load. For example I regularly create 100s of repos from a template repo via TF and I've not had any eventual consistency failures since the last bit of reworking (well over a year ago). I would question the validity of the current reports and suggest we work from first principles once the current code has been correctly optimised. |
|
Yeah, the API impact I didn't consider. This would add 1-2 API calls which user up Rate per repo creation (max 2, since EtagTransport should ensure that after first empty response further empty responses don't use rate) Maybe we could add the optional default false flag to enable this waiting to give the bug reports a way to try if that helps? |
I'm not 100% sure this even works at the moment. For v7 we should investigate using bored-engineer/github-conditional-http-transport so we don't need to explicitly manage etags at the resource level.
I really would leave this for now as a premature optimisation and focus on getting the resource into a better shape. The current create path not only uses multiple API calls in the create function, but it also calls update and read; this should be fixed before anything else is done. While we're at it we can figure out which functionality need to be moved into its own resource (e.g. vulnerabilities & pages). |
Supersedes #3034
Resolves #2604
Probably also resolves the following issues:
Resolves #897
Resolves #577
Resolves #2655
Resolves #2700
Resolves #2766
Before the change?
After the change?
maketarget forsweepPull request checklist
Schema migrations have been created if needed (example)Does this introduce a breaking change?
Please see our docs on breaking changes to help!