-
Notifications
You must be signed in to change notification settings - Fork 208
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
(feat) Bundles v2: Implement Bundle Interface Document #2922
Conversation
Signed-off-by: Sarah Christoff <schristoff@microsoft.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
/azp run porter-integration |
Azure Pipelines successfully started running 1 pipeline(s). |
Keeping Carolyn as a co-author on this for obvious reasons. Even months later, you're still the driving force behind this project @carolynvs ❤️ |
/azp run porter-integration |
Azure Pipelines successfully started running 1 pipeline(s). |
oh i already did that |
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.
This isn't necessarily about this PR, but you may want to consider starting spans for more functions. For example, creating a logger is nice, but the span is still the same span as from the parent (I believe from looking at the implementation of LoggerFromContext), so the attributed log entries on the span will not have the localization to the function as one might expect.
I don't have enough depth to give a functional review. However, there are a couple superficial things that might be good to update, and one more widespread thing to consider, granularity of spans.
|
||
for _, o := range c.Manifest.Outputs { | ||
addOutput(o) | ||
defName := c.addDefinition(sourceParam.Name, kind, sourceParam.Schema, defs) |
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.
defName := c.addDefinition(sourceParam.Name, kind, sourceParam.Schema, defs) | |
p.Definition = c.addDefinition(sourceParam.Name, kind, sourceParam.Schema, defs) |
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.
Right now I believe most of our stuff goes to stdOut, I would love to implement a structured logger so we could follow the span throughout multiple functions, appending more data, and being able to see exactly what func errors.
I believe right now because we abstract away our logging that implementing a structured logger wouldn't be a huge undertaking.
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.
Oh hey, this is new https://go.dev/blog/slog
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.
Structured log is good, but tracing is better. Tracing is structured logs + causality through call hierarchy. This is probably going to be more apparent in the operator as stuff will be less going to stdout and more to some telemetry ingestion pipeline.
What does this change
When setting a sharing mode is turned on for installations we need a way to share param/outputs/creds between them so we can wire them together.
The way we are doing this without updating the cnab spec is by converting everything into a document (type interface). This document type is something CNAB already implements - so once we get to translating porter yaml -> cnab json we get a smiley face.
Problem:
I remember talking to Carolyn about issues with using the existing
bundle.Parameters
andbundle.Credentials
and I recall it being about the required types needed passed into them (for example, path being required for parameters) but I don't remember where we left off with that.I guess we'll see what happens when we get there.
#2548
Checklist
Reviewer Checklist