-
Notifications
You must be signed in to change notification settings - Fork 4
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
PROP-27 - Implement Registry proxy #32
Conversation
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
cmd/proxy/main.go
Outdated
logger := slog.New(logHandler) | ||
slog.SetDefault(logger) | ||
|
||
mqttCfg := config.MQTTProxyConfig{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some stutter here in config MQTT Proxy Config
You can move the configs to the proxy package and import it as
mqttConfig := proxy.MQTTConfig{}
proplet/requests.go
Outdated
ID string | ||
FunctionName string | ||
WasmFile []byte | ||
WasmFileDownloadPath string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be changed to image URL?
proxy/service.go
Outdated
case chunk := <-s.dataChan: | ||
if err := s.mqttClient.PublishContainer(ctx, chunk); err != nil { | ||
s.logger.Error("failed to publish container chunk", | ||
"error", err, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use logger args
proxy/service.go
Outdated
|
||
s.logger.Info("published container chunk", | ||
"chunk_name", chunk.AppName, | ||
"chunk_no", chunk.ChunkIdx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
proxy/service.go
Outdated
continue | ||
} | ||
|
||
s.logger.Info("published container chunk", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be debug information?
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
test.md
Outdated
@@ -102,3 +102,28 @@ export PROPLET_THING_ID="" | |||
export PROPLET_THING_KEY="" | |||
propeller-proplet | |||
``` | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to absmach/propeller-docs#6 and delete it here
cmd/proxy/main.go
Outdated
LogLevel string `env:"PROXY_LOG_LEVEL" envDefault:"info"` | ||
|
||
BrokerURL string `env:"PROXY_MQTT_ADDRESS" envDefault:"tcp://localhost:1883"` | ||
PropletKey string `env:"PROXY_PROPLET_KEY,notEmpty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align with the other env variable config
PROPLET_CHANNEL_ID
PROPLET_THING_ID
PROPLET_THING_KEY
|
||
ChunkSize int `env:"PROXY_CHUNK_SIZE" envDefault:"512000"` | ||
Authenticate bool `env:"PROXY_AUTHENTICATE" envDefault:"false"` | ||
Token string `env:"PROXY_REGISTRY_TOKEN" envDefault:""` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need token and password or it is either one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its either of the 2
task/task.go
Outdated
@@ -35,6 +33,7 @@ type Task struct { | |||
ID string `json:"id"` | |||
Name string `json:"name"` | |||
State State `json:"state"` | |||
ImageURL URLValue `json:"image_url,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just string?
task/url.go
Outdated
@@ -0,0 +1,45 @@ | |||
package task | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Makefile
Outdated
if [[ ! "$$file" =~ \.wasm$$ ]]; then \ | ||
cp "$$file" $(GOBIN)/propeller-`basename "$$file"`; \ | ||
fi \ | ||
if [ "$$file" = "$${file%%.wasm}" ]; then \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use Makefile wildcard
directive here to contract the filtering and selection for copying?
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
cp "$$file" $(GOBIN)/propeller-`basename "$$file"`; \ | ||
fi \ | ||
done | ||
$(foreach f,$(wildcard $(BUILD_DIR)/*[!.wasm]),cp $(f) $(patsubst $(BUILD_DIR)/%,$(GOBIN)/propeller-%,$(f));) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible, in order to simplify handling of binaries, to have a subdir wasm
within build
dir, and put all Wasm binaries there. I guess these binaries are coming from examples, right? If really needed you can also add build/bin
and build/wasm
, but I do not think that we need build/bin
(as most of the time we'll need just Propeller services (and not examples), and they can live directly in the top build
dir, for convenience), but if this simplifies copying, then please go ahead and make both build/bin
and build/wasm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, as you would need to prefix each bin anyway - I will merge as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible, in order to simplify handling of binaries, to have a subdir
wasm
withinbuild
dir, and put all Wasm binaries there. I guess these binaries are coming from examples, right? If really needed you can also addbuild/bin
andbuild/wasm
, but I do not think that we needbuild/bin
(as most of the time we'll need just Propeller services (and not examples), and they can live directly in the topbuild
dir, for convenience), but if this simplifies copying, then please go ahead and make bothbuild/bin
andbuild/wasm
.
We can implement this .
What type of PR is this?
What does this do?
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Did you document any new/modified features?
Notes