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: builds helm chart #137

Merged
merged 24 commits into from
Oct 22, 2022
Merged

feat: builds helm chart #137

merged 24 commits into from
Oct 22, 2022

Conversation

AlexsJones
Copy link
Member

@AlexsJones AlexsJones commented Oct 20, 2022

This PR is in two parts

First part This PR is to implement the helm chart and simplify the generation of that chart

The second part will be to serve the chart from the repo

After this is accepted I will add the code to convert this repo into a helm repo which will enable

helm repo add ofo https://github.com/open-feature/open-feature-operator
helm upgrade --install release ofo/open-feature-operator

for now you can use

#clone the git repo 
cd chart
helm upgrade ofo . --install

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2022

Codecov Report

Merging #137 (7dc9b1e) into main (80bcfba) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #137   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          1       1           
  Lines        122     122           
=====================================
  Misses       122     122           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Makefile Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member

toddbaert commented Oct 20, 2022

for now you can use

#clone the git repo 
cd chart
helm upgrade --install . ofo 

That gets me Error: release name is invalid: . on helm v3.10.0.

I think you mean helm upgrade ofo . --install,

However, the ofo-chart-controller-manager stays in a ContainerCreating state indefinitely, and streaming the logs fails. Maybe I've missed a step?

chart/Chart.yaml Outdated Show resolved Hide resolved
@AlexsJones
Copy link
Member Author

Fixed, this was due to missing CRD's and wrong image

@AlexsJones
Copy link
Member Author

image

.release-please-manifest.json Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
chart/Chart.yaml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
chart/Chart.yaml Show resolved Hide resolved
@toddbaert
Copy link
Member

I'll test this again in a couple hours when I'm back at my PC!

@toddbaert toddbaert self-requested a review October 21, 2022 19:26
@toddbaert
Copy link
Member

toddbaert commented Oct 21, 2022

Thanks!

This all works for me provided I install cert-manager first, otherwise I'm missing objects.

Error: unable to build kubernetes objects from release manifest: [resource mapping not found for name: "ofo-serving-cert" namespace: "" from "": no matches for kind "Certificate" in version "cert-manager.io/v1"

I'm know this is a pre-requisite, and not a new one, but I'm still a bit confused if this hard dependency is a good idea. Could the cert manager requirement prevent adoption if an admin has some alternative policy or solution for generating self-signed certs? Is it more or less standard for operators that accept requests to require cert-manager? I'm not truly not sure, so I defer once more to your expertise, @AlexsJones !

@toddbaert
Copy link
Member

@AlexsJones I pushed a commit to add a couple files to the release please extra-files member, which tells release please to look for the x-release-please annotations in those files. Hope you don't mind.

README.md Show resolved Hide resolved
@AlexsJones
Copy link
Member Author

nfused if this hard dependency is a good idea. Could the cert manager requirement prevent adoption if an admin has some alternative policy or

For posterity, the reason it needs the certificate is for mTLS between the api-server and the webhook.
It's something we do need, it doesn't have to be cert-manager, but given they just got brought into CNCF incubation I think we're backing the right horse

AlexsJones and others added 6 commits October 22, 2022 20:08
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
AlexsJones and others added 13 commits October 22, 2022 20:08
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Switching to use release-please comment

Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Co-authored-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Alex Jones <alex.jones@canonical.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: Alex Jones <alexsimonjones@gmail.com>
@AlexsJones
Copy link
Member Author

Please review when you have a spare moment, I added to the readme @toddbaert
Also I will squash all these commits, I was too lazy to rebase so did a skip

@AlexsJones AlexsJones merged commit 1525421 into main Oct 22, 2022
@AlexsJones AlexsJones deleted the feature/helm branch October 22, 2022 19:37
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Readme changes look good to me!

Sorry for that missing comma!

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.

5 participants