✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	279fcc9
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/6877b0ec5647fe000808fe33
😎 Deploy Preview	https://deploy-preview-2099--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

I think this is a good start, but we should also do this for catalogd...

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.48%. Comparing base (b25356d) to head (279fcc9).
Report is 3 commits behind head on main.

@@           Coverage Diff           @@
##             main    #2099   +/-   ##
=======================================
  Coverage   73.48%   73.48%           
=======================================
  Files          78       78           
  Lines        7240     7240           
=======================================
  Hits         5320     5320           
  Misses       1568     1568           
  Partials      352      352           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I think this is a good start, but we should also do this for catalogd...

@tmshort I've added it. But are you sure we aren't jumping the gun a little. I wonder if we'll ever add new APIs or controllers to catalogd...:thinking:

Experimental controllers should carry //go:build !standard build tag. This will exclude it from the standard manifest generation step in the Makefile.

This statement doesn't apply. We do not build separately for standard vs experimental; it's the same code, just different manifests. (We literally use the same build image.) This means that any new CRDs will have to be provided regardless of standard v. experimental, even if they only define an empty Spec and Status for their standard definition.

EDIT: To clarify, experimental controllers should be included/excluded based on feature gate flags, not build flags. The manifests (including CRDs) can change at any time during a deployment (including argument flags).

EDIT 2: However, i think what's not clear (to me, at least initially) is that this tag is only used for generating the RBAC, and is not used during code compilation. I will likely need to update the RFCs to make the use of //go:build !standard clear. We may want to consider the adding the term "rbac" to this tag to make the use even more clear, and possibly do the same for webhooks in the future. Regardless, my confusion over the use of the build tag needs to be resolved via documentation or similar, because it will confuse someone else!

@grokspawn believes we might, and I would prefer a complete solution.

It's a good sign that the committed manifests did not change!

This looks good, but I don't know why config/webhook/manifests.yaml was added.

Experimental controllers should carry //go:build !standard build tag. This will exclude it from the standard manifest generation step in the Makefile.

This statement doesn't apply. We do not build separately for standard vs experimental; it's the same code, just different manifests. (We literally use the same build image.) This means that any new CRDs will have to be provided regardless of standard v. experimental, even if they only define an empty Spec and Status for their standard definition.

EDIT: To clarify, experimental controllers should be included/excluded based on feature gate flags, not build flags. The manifests (including CRDs) can change at any time during a deployment (including argument flags).

EDIT 2: However, i think what's not clear (to me, at least initially) is that this tag is only used for generating the RBAC, and is not used during code compilation. I will likely need to update the RFCs to make the use of //go:build !standard clear. We may want to consider the adding the term "rbac" to this tag to make the use even more clear, and possibly do the same for webhooks in the future. Regardless, my confusion over the use of the build tag needs to be resolved via documentation or similar, because it will confuse someone else!

@tmshort
The binaries should be built including everything (and we do the branching via feature gates as you mentioned in a previous comment). The !standard artifice is strictly to remove experimental rbac from the standard manifests. So, I think we're aligned on that =D

I think we can get away with not generating new and experimental CRDs by not adding them to the standardKinds map but still adding them to the generate-crds.sh lists of things to generate. I would highly encourage us to use this path (unless there's something I'm not seeing) because it feels a bit weird to ship empty CRDs to production. I've done this in the Boxcutter WIP PR if you want to have a look to see what that looks like in code.

I've put an additional commit to add exp/std split support for the catalogd webhooks. Feels like it leads to a lot of duplication (and I'd also pose the same question of whether we have any inclination to have experimental webhooks, etc.).

Regarding catalogd, I think the solution is complete without this additional complexity under the assumption there will be no new controllers and we'll just have experimental changes to the existing CRD and add features to the existing controllers. I get that there's a certain lack of symmetry, but I think we'd be carrying around additional complexity for no added benefit, or just in case future us decide to violate the stated assumption - and it should be easy enough to add the split when the time comes anyway. But, I'll defer to your better judgement here since you've put all of this together.

@tmshort
The binaries should be built including everything (and we do the branching via feature gates as you mentioned in a previous comment). The !standard artifice is strictly to remove experimental rbac from the standard manifests. So, I think we're aligned on that =D

Exactly.

I think we can get away with not generating new and experimental CRDs by not adding them to the standardKinds map but still adding them to the generate-crds.sh lists of things to generate. I would highly encourage us to use this path (unless there's something I'm not seeing) because it feels a bit weird to ship empty CRDs to production. I've done this in the Boxcutter WIP PR if you want to have a look to see what that looks like in code.

Yes, that is the purpose of those lists in the generator.

I've put an additional commit to add exp/std split support for the catalogd webhooks. Feels like it leads to a lot of duplication (and I'd also pose the same question of whether we have any inclination to have experimental webhooks, etc.).

I hear you about duplication. However, we still need to keep the manifest generation separate for downstream reasons. So, we unfortunately have to deal with it. Also, there are kubebuilder limitations on the number of paths it can accept in a single invocation (it's only one path).

Regarding catalogd, I think the solution is complete without this additional complexity under the assumption there will be no new controllers and we'll just have experimental changes to the existing CRD and add features to the existing controllers. I get that there's a certain lack of symmetry, but I think we'd be carrying around additional complexity for no added benefit, or just in case future us decide to violate the stated assumption - and it should be easy enough to add the split when the time comes anyway. But, I'll defer to your better judgement here since you've put all of this together.

I would prefer to have it in place for catalogd, because when the time comes around, and something is added to catalogd, and the experimental/standard split is only partially done (i.e. for CRDs but not RBAC), it going to really confuse someone, and they are going to have to recreate this infrastructure.

You have done the work for this, so thank you!

@tmshort
No worries. If you're good with the way it is, then let's proceed =D

/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tmshort

The full list of commands accepted by this bot can be found here.

The pull request process is described here

This does not appear to have any downstream issues...
/lgtm