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

[issue-172] Adds flag success-build-history-limit and failed-build-history-limit #334

Merged
merged 2 commits into from
Sep 1, 2023

Conversation

fcaroline2020
Copy link
Contributor

@fcaroline2020 fcaroline2020 commented Aug 2, 2023

  • Adds flag success-build-history-limit and failed-build-history-limit to image create, save, patch
  • both default to 10 to keep the behavior the same as before these flags were added

resolves #172
resolves #244

@fcaroline2020 fcaroline2020 requested a review from a team as a code owner August 2, 2023 09:26
@chenbh
Copy link
Contributor

chenbh commented Aug 28, 2023

I actually think we shouldn't set a default via the CLI, if the flag isn't provided, we should leave that field null.

My opinion is that defaults should only be applied in one place, and that place is usually the kpack webhook controller. This way, the actual value can differ between kpack versions and in the distant future might even be configurable at the cluster level without ever having to upgrade the CLI.

@fcaroline2020 fcaroline2020 force-pushed the issue-172-support-build-history-limit-flag branch 2 times, most recently from abfac5a to 9e68bfe Compare August 29, 2023 16:03
@fcaroline2020
Copy link
Contributor Author

To set values to null I had to change the success-build-history-limit and failed-build-history-limit from int64 to string (otherwise I would have had to pick an arbitrary default value like -1 which then showed up in the documentation).

pkg/commands/image/create.go Outdated Show resolved Hide resolved
pkg/image/factory.go Outdated Show resolved Hide resolved
@fcaroline2020 fcaroline2020 force-pushed the issue-172-support-build-history-limit-flag branch from d171c9d to 5e0f509 Compare August 31, 2023 09:54
@fcaroline2020 fcaroline2020 reopened this Aug 31, 2023
@fcaroline2020 fcaroline2020 force-pushed the issue-172-support-build-history-limit-flag branch from 5bde8d1 to 3f884dd Compare August 31, 2023 10:55
@chenbh
Copy link
Contributor

chenbh commented Aug 31, 2023

@fcaroline2020 needs rebasing

@fcaroline2020 fcaroline2020 force-pushed the issue-172-support-build-history-limit-flag branch from 3f884dd to 7c4fe51 Compare September 1, 2023 11:51
@chenbh chenbh merged commit 8465fc9 into main Sep 1, 2023
1 check passed
@chenbh chenbh deleted the issue-172-support-build-history-limit-flag branch September 1, 2023 20:02
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.

Add support for build history limit configuration Support modifying buildhistorylimit on the image resource
2 participants