-
Notifications
You must be signed in to change notification settings - Fork 35
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
Java app build extension #348
base: main
Are you sure you want to change the base?
Conversation
This uses an event to run the mvnw build (or it can be customised via options), so it is only used when the app host is running the resource Fixes #339
Minimum allowed line rate is |
State = new($"mvnw exited with {mvnw.ExitCode}", KnownResourceStates.FailedToStart) | ||
}).ConfigureAwait(false); | ||
|
||
throw new InvalidOperationException($"mvnw build failed with exit code {mvnw.ExitCode}"); |
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.
Why throw?
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.
It's how I've seen other hooks implemented in the past.
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.
Where does this show up? Wouldn't it be better to show it in the dashboard? Also what happens when the build fails? Do I need to restart the entire apphost to rebuild or can I trigger it from the dashboard? (custom command)
Worth thinking through that I think.
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.
The notification service reporting shows up on the dashboard and I think the exception bubbles to the console of the resource that it was relative for.
A custom command would be a good idea to run a rebuild, I'll add that.
private static IResourceBuilder<JavaAppExecutableResource> WithJavaDefaults( | ||
this IResourceBuilder<JavaAppExecutableResource> builder, | ||
JavaAppExecutableResourceOptions options) => | ||
builder.WithOtlpExporter() |
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.
Does this work without making https work?
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.
Without HTTPS on the dashboard?
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.
Yea does this work right now?
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.
I assume so 🤣
This uses an event to run the mvnw build (or it can be customised via options), so it is only used when the app host is running the resource
Closes #339
PR Checklist
Other information