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

feat: Add support to use ETag for conditional requests against the Github API #6505

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

feat: Add support to use ETag for conditional requests against the Github API #6505

wants to merge 7 commits into from

Conversation

andrewhibbert
Copy link

@andrewhibbert andrewhibbert commented Jan 31, 2025 •
edited
Loading

Adds functionality to use conditional requests with the Github API by adding etag to request headers. If the response has not changed a 304 response is returned, which does not count against the primary rate limit.

Checklist

  • [N/A] When introducing a new scaler, I agree with the scaling governance policy
  • I have verified that my change is according to the deprecations & breaking changes policy
  • Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements
  • [N/A] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #6503

Relates to kedacore/keda-docs#1532

@andrewhibbert andrewhibbert requested a review from a team as a code owner January 31, 2025 20:32
@andrewhibbert andrewhibbert changed the title Feat etags feat: Add support to use ETag for conditional requests against the Github API Jan 31, 2025
JorTurFer
JorTurFer reviewed Feb 3, 2025
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

This feature looks super nice! Thanks for the improvement 😄
Could you enable the etag feature in one of the github e2e test cases to keep it covered with e2e tests? I don't think that we need to add another fully e2e test case but just update one of the already existing to include this feature.
We'd need that you open a PR to docs adding this new parameter to the scaler docs 🙏

@andrewhibbert andrewhibbert mentioned this pull request Feb 4, 2025
Open
@andrewhibbert
Copy link
Author

This feature looks super nice! Thanks for the improvement 😄 Could you enable the etag feature in one of the github e2e test cases to keep it covered with e2e tests? I don't think that we need to add another fully e2e test case but just update one of the already existing to include this feature. We'd need that you open a PR to docs adding this new parameter to the scaler docs 🙏

Thanks. I have opened a PR in the docs repo and changed one of the e2e testcase to have enabledEtags set to true

@JorTurFer
Copy link
Member

JorTurFer commented Feb 4, 2025 •
edited by github-actions bot
Loading

/run-e2e github
Update: You can check the progress here

@JorTurFer JorTurFer mentioned this pull request Feb 4, 2025
Open
23 tasks
zroubalik
zroubalik reviewed Feb 6, 2025
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks! Let's just fix the semgrep and we shoud be good to merge

pkg/scalers/github_runner_scaler_test.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

@zroubalik zroubalik zroubalik approved these changes

@JorTurFer JorTurFer JorTurFer 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.

Use conditional requests functionaliy with Github Scaler
3 participants
@andrewhibbert @JorTurFer @zroubalik