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

spinapp: Relax executor validation #308

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

endocrimes
Copy link
Contributor

I've been using the Spin Operator with Flux deployments, and requring executors to exist before creating a Spin App leads to some... rather un-ergonomic requirements, by nature of how resource installations need to be tiered when orchestrating tools like Flux:

Specifically:

  1. Install dependencies (cert-manager)
  2. Install Spin Operator
  3. Install executor
  4. finally install application

This seems reasonable enough at first, but because executors and applications are in the same domain (namespace), it's more awkward than one would otherwise expect, because every application suddenly becomes two sets of packages rather than one.

By relaxing this requirement, we still provide feedback to users (events), but allow those to be collapsed back into a single installation step - which is a great improvement for the simplest-possible case.

I've been using the Spin Operator with Flux deployments, and requring
executors to exist before creating a Spin App leads to some... rather
un-ergonomic requirements, by nature of how resource installations need
to be tiered when orchestrating tools like Flux:

Specifically:

1) Install dependencies (cert-manager)
2) Install Spin Operator
3) Install executor
4) finally install application

This seems reasonable enough at first, but because executors and
applications are in the same domain (namespace), it's more awkward than
one would otherwise expect, because every application suddenly becomes
_two_ sets of packages rather than one.

By relaxing this requirement, we still provide feedback to users
(events), but allow those to be collapsed back into a single
installation step - which is a great improvement for the
simplest-possible case.
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.

Overall I'm on board for this. I do like the idea of making it produce warnings though.

@@ -33,45 +16,3 @@ func TestValidateReplicas(t *testing.T) {
fldErr = validateReplicas(spinv1alpha1.SpinAppSpec{Replicas: 1})
require.Nil(t, fldErr)
}

func TestValidateAnnotations(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we're getting rid of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They only make sense in the context of an executor right now - which when it's not necessarily there already, would cause inconsistent validation

@calebschoepp calebschoepp added this to the v0.4.0 milestone Sep 6, 2024
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.

2 participants