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

add private image support #68

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

add private image support #68

wants to merge 4 commits into from

Conversation

michelleN
Copy link
Contributor

@michelleN michelleN commented Feb 15, 2024

This PR is a temporary fix for a templating error described in #66 and it also adds support for working with private spin-operator images.

Copy link

This PR now has an image available for testing:

  ttl.sh/spoopy-operator-pr-68:24h

@michelleN michelleN changed the title make default namespace spin-operator-system make default ns spin-operator-system, private image support Feb 15, 2024
@rajatjindal
Copy link
Contributor

Hi @michelleN, with #69 merged, do we still need this PR? (or is it something completely different?)

@michelleN
Copy link
Contributor Author

@rajatjindal - I'll remove the ns related commit then this PR will only be related to supporting private images

@michelleN michelleN changed the title make default ns spin-operator-system, private image support add private image support Feb 15, 2024
@michelleN michelleN requested a review from vdice February 15, 2024 16:47
@@ -97,7 +102,7 @@ helm upgrade spin-operator \
--namespace spin-operator \
--devel \
--wait \
oci://ghcr.io/spinkube/spin-operator
charts/spin-operator
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we have published charts now, should I take out this change pointing to local path here? @vdice

Copy link
Contributor

Choose a reason for hiding this comment

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

Good Q... we do have published prerelease charts now but I can also understand just keeping references local until we have a first official (eg v0.1.0) chart release. As an aside, if we use this updated local reference, we can remove --devel (and the TODO note above.)

Copy link
Contributor

@vdice vdice left a comment

Choose a reason for hiding this comment

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

ImgPullSecret functionality worked well for me! Some comments/Qs...

charts/spin-operator/README.md Outdated Show resolved Hide resolved
@@ -97,7 +102,7 @@ helm upgrade spin-operator \
--namespace spin-operator \
--devel \
--wait \
oci://ghcr.io/spinkube/spin-operator
charts/spin-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Good Q... we do have published prerelease charts now but I can also understand just keeping references local until we have a first official (eg v0.1.0) chart release. As an aside, if we use this updated local reference, we can remove --devel (and the TODO note above.)

@@ -65,6 +65,11 @@ The spin-operator chart currently includes the following sub-charts:

### Installing the Chart

If the spin operator image you are going to install lives in a private repository, please first create a Kubernetes secret called `ghcr-credentials` in the
`spin-operator-system` namespace before installing the helm chart below. See the [private-images](#private-images) section below for instructions on how to
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double-checking that this spin-operator-system namespace is intended. In the helm install cmds below, spin-operator is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. will update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an update here is still needed.

Makefile Outdated Show resolved Hide resolved
charts/spin-operator/templates/deployment.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@michelleN
Copy link
Contributor Author

thanks @vdice - updated based on your feedback.

Copy link
Contributor

@vdice vdice left a comment

Choose a reason for hiding this comment

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

One last update requested, otherwise LGTM!

@@ -65,6 +65,11 @@ The spin-operator chart currently includes the following sub-charts:

### Installing the Chart

If the spin operator image you are going to install lives in a private repository, please first create a Kubernetes secret called `ghcr-credentials` in the
`spin-operator-system` namespace before installing the helm chart below. See the [private-images](#private-images) section below for instructions on how to
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an update here is still needed.

@lann
Copy link
Contributor

lann commented Feb 19, 2024

Does the admin workstation also need to be authed to the private repo or does that all go through the cluster for helm install?

@michelleN
Copy link
Contributor Author

@lann it all goes through the cluster

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@michelleN
Copy link
Contributor Author

@endocrimes thanks. updated.

Copy link
Contributor

@calebschoepp calebschoepp left a comment

Choose a reason for hiding this comment

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

I'll defer to others with more experience in this area but I don't see any glaring issues.

michelleN and others added 2 commits March 5, 2024 11:53
+ update the Chart README
+ add imagePullSecrets patch to kustomize templates
+ adding patch auto generated same updates to helm
deployment template
+ add make target for generated container registry
secret

Signed-off-by: Michelle Dhanani <michelle@fermyon.com>
Co-authored-by: Danielle <dani@builds.terrible.systems>
michelleN and others added 2 commits March 5, 2024 11:53
Co-authored-by: Danielle <dani@builds.terrible.systems>
Signed-off-by: Michelle Dhanani <michelle@fermyon.com>
@michelleN
Copy link
Contributor Author

Since image will be public today, I don't think we need to merge the config updates the way it's done here. We should probably add an optional value to the operator deployment chart for registry credentials. The make target might be helpful but I think it'd be more useful in script under hack/

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 imagePullSecret to the spin operator deployment yaml in helm chart
6 participants