[DevEx] xpkg: add error contexts and print & fail on parse errors #342
Conversation
sttts
force-pushed
the
sttts-xpkg-error-contexts
branch
2 times, most recently
from
5d6608a
to
5468781
Compare
There was a problem hiding this comment.
Thanks @sttts ! I love all of the improvements to the amount of context we're providing. I left a couple of questions below. Let me know if there's anything I can clarify 👍
| @@ -115,6 +115,35 @@ type buildCmd struct { | |||
| Ignore []string `help:"Paths, specified relative to --package-root, to exclude from the package."` | |||
| } | |||
|
|
|||
| func (c *buildCmd) Help() string { | |||
There was a problem hiding this comment.
nit - It seems like this output is more about the xpkg command and less about building the actual artifact, is that accurate? Would it make sense to move this up to that command and inside this subcommand add more details about the build operation?
There was a problem hiding this comment.
am actually calling this in the xpkg command. But let me reread and maybe rephrase one of them.
There was a problem hiding this comment.
Please take a look at the new text for the build command. You will probably have more background of the command, so feel free to suggest another text, fix or add things.
|
|
||
| If a package is specified, it will be added to the crossplane.yaml file | ||
| in the current directory. | ||
| ` |
There was a problem hiding this comment.
Would it be worth adding interaction details in addition to the above? I'm thinking similar to what is in code comments like so
Lines 25 to 27 in 75dffb2
internal/xpkg/workspace/workspace.go
Outdated
| // TODO(hasheddan): we cannot make use of strict unmarshal to identify | ||
| // extraneous fields because we don't have the underlying Go types. In | ||
| // the future, we would like to provide warnings on fields that are | ||
| // extraneous, but we will likely need to augment the OpenAPI validation | ||
| // to do so. | ||
| if err := k8syaml.Unmarshal(b, &obj); err != nil { | ||
| return NodeIdentifier{}, err | ||
| if v.printer != nil { | ||
| v.printer.Printfln("WARNING: ignoring file %s: missing 'kind' field, not a Kubernetes object", v.relativePath(pCtx.path)) |
There was a problem hiding this comment.
I believe this path is invoked both from xpkg build as well from xpls during document parsing. It's unclear how this error will be rendered in the later case, maybe it'll show up in the xpls logs. I wonder if it's still worth returning error instead of nil, although given
up/internal/xpkg/workspace/workspace.go
Lines 215 to 216 in 75dffb2
There was a problem hiding this comment.
Added error aggregation here too now.
There was a problem hiding this comment.
To understand: for the language server we want to be more permissive with errors. When the user edits files, we should try to keep going in parsing. Thinking about adding a permissive flag that ignores errors, just warns.
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
sttts
force-pushed
the
sttts-xpkg-error-contexts
branch
from
b564af8
to
f26cf68
Compare
|
|
There was a problem hiding this comment.
Thanks @sttts, just gave this PR a try for:
upbound/provider-aws:up xpkg batchis producing provider families with the expected layers and metadata. Observed with:index.docker.io/ulucinar/provider-family-aws:v0.37.0-rc.0.56.g257bedd2,index.docker.io/ulucinar/provider-aws-ec2:v0.37.0-rc.0.56.g867782ef,index.docker.io/ulucinar/provider-aws:v0.37.0-rc.0.56.g867782efupbound/provider-azuread:up xpkg buildandup xpkg pushare producing provider packages with the expected layers and metadata. Observed with:index.docker.io/ulucinar/provider-azuread:v0.10.0-rc.0.6.gb7fc503
Thank you for the improvements.
|
Thanks @ulucinar. Let's 🚢 it. |
This PR adds errors contexts (e.g. filenames) to errors returned in the xpkg commands. Moreover, it removes the TODO in the workspace loader of missing error handling.