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 queue size limit to github webhook server helm template #1712

Merged
merged 7 commits into from
Aug 23, 2022

Conversation

Sajadorouji
Copy link
Contributor

We recently hit the queue size limit that is 100 by default and as i find out there is no way to pass the value through the helm chart

Here is a fix to enable users to add their queue size limit in case of need

Copy link
Collaborator

@toast-gear toast-gear left a comment

Choose a reason for hiding this comment

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

LGTM beyond the docs need updating charts/actions-runner-controller/readme.md

@Sajadorouji
Copy link
Contributor Author

Hey thanks for the review
Readme.md file has been updated with the correct name and value

@Sajadorouji Sajadorouji requested a review from toast-gear August 15, 2022 07:49
@Sajadorouji
Copy link
Contributor Author

@debugger24 @mumoshu Thanks for the review and comments
I've commit your suggestion on the helm value, do we need to bump the chart version as well or not

@Sajadorouji Sajadorouji requested a review from debugger24 August 17, 2022 08:42
debugger24
debugger24 previously approved these changes Aug 17, 2022
Copy link
Collaborator

@toast-gear toast-gear left a comment

Choose a reason for hiding this comment

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

Apologies I've not been on this, work is busy.

I am a believer of convention over configuration. We historically have not put the default in the values.yaml docs if it's set in the app itself. Perhaps that was mistake but I'd prefer for us to be consistent so could we:

  1. remove the stated default from the docs as it's inconsistent with the rest of the doc
  2. remove ## Queue limit size override, by default its 100 from the values.yaml as this is the same thing as point 1

I'm fine with the commented out queueLimit: 100 in the values.yaml

@Sajadorouji
Copy link
Contributor Author

@toast-gear I've updated the value file and deleted the comment also enabled the queuelimit value 100 to be active
But i didn't understand your point 1 in the comment
If we leave this enable in value file with 100 that would be same as the readme description, if i'm missing something please point me to it

@Sajadorouji Sajadorouji requested review from toast-gear and debugger24 and removed request for toast-gear and debugger24 August 22, 2022 10:22
Copy link
Collaborator

@toast-gear toast-gear left a comment

Choose a reason for hiding this comment

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

LGTM

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 23, 2022

Thanks a lot for your contribution! @Sajadorouji ☺️
Regarding the comment-out of the default value in values.yaml, I think that's probably the way to go for us, as it should result in less issues when e.g. we updated the default value in Go code but forgot to do so in values.yaml.

However, we do have some inconsistencies in other values within the same file, like, we probably want to comment out the default syncPeriod setting as well for the same reason and for consistency.

@mumoshu mumoshu merged commit ec58ad1 into actions:master Aug 23, 2022
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.

4 participants