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

pubsub krm #36

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

pubsub krm #36

wants to merge 20 commits into from

Conversation

Akshaya-T
Copy link
Contributor

@Akshaya-T Akshaya-T commented Feb 8, 2023

@Akshaya-T Akshaya-T changed the title pubsub krm with versioned API processor pubsub krm Feb 9, 2023
@@ -15,7 +15,8 @@ COPY cmd/${FUNCTION}/*.go .
RUN --mount=type=cache,target=/root/.cache/go-build go build -mod readonly -v -o /usr/local/bin/config-function ./

RUN controller-gen crd paths=./pkg/workloads output:crd:dir=crd/workloads && \
Copy link
Owner

Choose a reason for hiding this comment

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

shouldn't need to hardcode individual directories, make a make directive to do this, so that it can work both within and outside of the container.

@@ -16,6 +16,7 @@ build: check-function-var
crd: check-function-var
controller-gen crd paths=./pkg/workloads output:crd:dir=crd/workloads
controller-gen crd paths=./pkg/pgbouncer output:crd:dir=crd/pgbouncer
controller-gen crd paths=./pkg/pubsub output:crd:dir=crd/pubsub
Copy link
Owner

Choose a reason for hiding this comment

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

oh, this exists. we shouldn't have to hardcode, and we should be able to call this from within the dockerfile. If we have different implementations for the same thing, it is error prone and confusing.

@@ -0,0 +1,35 @@
package main
Copy link
Owner

Choose a reason for hiding this comment

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

let's start figuring out how we can generate these.

)

// +kubebuilder:object:root=true
type PubsubSubscription struct {
Copy link
Owner

Choose a reason for hiding this comment

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

there's some krm function type that we have to eventually use

Config PubSubConfig `json:"config,omitempty"`
}

func (pubSubConfig *PubSubConfig) fill_defaults() {
Copy link
Owner

Choose a reason for hiding this comment

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

  1. use camelcase
  2. don't call it this, this is basically a 'constructor', so they should be called something like 'NewSubscription' if it is returning a reference to a initialised subscription object.

@@ -0,0 +1,14 @@
package fnutils
Copy link
Owner

Choose a reason for hiding this comment

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

not in the fnutil path because it doesn't have anything to do with krm. Maybe we'll rename fnutils to krm or something.

Copy link
Owner

Choose a reason for hiding this comment

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

you can just put this in common

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