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: configurable imagePullPolicy via Helm #740

Merged
merged 15 commits into from
Feb 3, 2023

Conversation

sudiptob2
Copy link
Member

@sudiptob2 sudiptob2 commented Feb 1, 2023

Fixes #721

Required tasks

  • value file contains a new value imagePullPolicy
  • the value is patched to all the deployments in our helm overlay (modify this )
  • default value set to Always
  • Run ./.github/scripts/generate-helm-docs.sh to update the helm chart docs
  • Add readme-generator-for-helm in gitignore

@sudiptob2 sudiptob2 marked this pull request as ready for review February 1, 2023 18:00
@sudiptob2
Copy link
Member Author

I am pretty beginner in keptn. I would appreciate your valuable comment on the PR weather it is on the right track or not. 💯

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #740 (054d858) into main (bab6a6f) will increase coverage by 0.05%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #740      +/-   ##
==========================================
+ Coverage   57.69%   57.75%   +0.05%     
==========================================
  Files          91       91              
  Lines        7243     7243              
==========================================
+ Hits         4179     4183       +4     
+ Misses       2894     2891       -3     
+ Partials      170      169       -1     
Impacted Files Coverage Δ
...lers/lifecycle/keptnworkloadinstance/controller.go 82.80% <0.00%> (+1.80%) ⬆️
Flag Coverage Δ
component-tests 48.56% <ø> (+0.62%) ⬆️
keptn-lifecycle-operator 53.60% <ø> (ø)
klt-cert-manager 67.50% <ø> (ø)
scheduler 21.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@sudiptob2
Copy link
Member Author

@thisthat @thschue awaiting a review on this PR

@RealAnna
Copy link
Contributor

RealAnna commented Feb 2, 2023

Thank you for the contribution @sudiptob2 LGTM only one thing to do still: our helm docs are autogenerated, so any time you add one new value you need to run the script in ./.github/scripts/generate-helm-docs.sh to re-generate the docs.

Sorry we forgot to mention that in the ticket 😸

@sudiptob2
Copy link
Member Author

@RealAnna Thanks much for helping me out, I am going to run that script and make another commit.
One this is to mention is that This ticket is the most detailed ticket I have ever found 🚀 🚀

@sudiptob2
Copy link
Member Author

Could you please please mention where the script is, I can not find the generate-helm-docs.sh file in the project @RealAnna

@RealAnna
Copy link
Contributor

RealAnna commented Feb 2, 2023

Could you please please mention where the script is, I can not find the generate-helm-docs.sh file in the project @RealAnna

https://github.com/keptn/lifecycle-toolkit/blob/main/.github/scripts/generate-helm-docs.sh :)

@sudiptob2
Copy link
Member Author

Oh Its in the kep

Could you please please mention where the script is, I can not find the generate-helm-docs.sh file in the project @RealAnna

https://github.com/keptn/lifecycle-toolkit/blob/main/.github/scripts/generate-helm-docs.sh :)

Yes, I also found it in the repo, but not in my version. understood, I have to sync the fork. It's been added recently I guess. Thanks a lot @RealAnna

@sudiptob2
Copy link
Member Author

I have a confusion, in the generate-helm-docs.sh we pull a doc generator project in the current directory ref
Should I add this directory to gitignore ? @RealAnna

@RealAnna
Copy link
Contributor

RealAnna commented Feb 2, 2023

I have a confusion, in the generate-helm-docs.sh we pull a doc generator project in the current directory ref Should I add this directory to gitignore ? @RealAnna
@sudiptob2 yes it can be nice to add it into the git ingore indeed, thanks for noticing!

@sudiptob2 sudiptob2 force-pushed the feat/721/configurable-pull-policy branch from 23f07fd to 3882974 Compare February 2, 2023 12:17
@sudiptob2 sudiptob2 requested a review from RealAnna February 2, 2023 12:21
RealAnna
RealAnna previously approved these changes Feb 2, 2023
Copy link
Contributor

@RealAnna RealAnna left a comment

Choose a reason for hiding this comment

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

LGTM!

thisthat
thisthat previously approved these changes Feb 2, 2023
.gitignore Outdated Show resolved Hide resolved
@sudiptob2 sudiptob2 dismissed stale reviews from thisthat and RealAnna via b5864a5 February 2, 2023 12:55
@sudiptob2 sudiptob2 requested review from thisthat and RealAnna and removed request for thisthat February 2, 2023 13:01
@odubajDT
Copy link
Contributor

odubajDT commented Feb 2, 2023

I already reverted the requested changes, sorry one more time for taking you to the wrong path and thanks for contribution!

@sudiptob2
Copy link
Member Author

I already reverted the requested changes, sorry one more time for taking you to the wrong path and thanks for contribution!

Thanks much all, can't belive I am able to make a contribution even as a beginner. 🚀 Really appreciate your help @odubajDT Looking forward to do more

@sudiptob2
Copy link
Member Author

hi, @odubajDT @RealAnna still one pipeline check is failing. Could you please suggest to me the next step?

@sonarcloud
Copy link

sonarcloud bot commented Feb 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sudiptob2
Copy link
Member Author

I updated the branch with main, now I guess all checks are passed. However please let me know if I have to take any further steps.

@sudiptob2 sudiptob2 requested review from RealAnna and thisthat and removed request for RealAnna and thisthat February 2, 2023 17:46
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @sudiptob2 for your contribution 🙌

@thisthat thisthat merged commit b6b4160 into keptn:main Feb 3, 2023
@keptn-bot keptn-bot mentioned this pull request Feb 3, 2023
@sudiptob2 sudiptob2 deleted the feat/721/configurable-pull-policy branch February 7, 2023 16:22
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.

Make Image Pull Policy configurable via Helm value
5 participants