Skip to content

Add build log to deployment notification email #631

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Add build log to deployment notification email #631

wants to merge 4 commits into from

Conversation

miles-grant-ibigroup
Copy link
Contributor

To accomplish this, I've added a new redirect-capable artifact download endpoint

br648
br648 reviewed Jul 15, 2025
View reviewed changes
src/main/java/com/conveyal/datatools/manager/controllers/api/DeploymentController.java Outdated
@@ -602,6 +607,8 @@ public static void register (String apiPrefix) {
options(apiPrefix + "secure/deployments", (q, s) -> "");
get(apiPrefix + "secure/deployments/:id/download", DeploymentController::downloadDeployment);
get(apiPrefix + "secure/deployments/:id/artifact", DeploymentController::downloadBuildArtifact);
// This path allows the downloaded artifact file to be named anything
get(apiPrefix + "secure/deployments/:id/artifact/:ignored", DeploymentController::downloadBuildArtifact);
Copy link
Contributor

Choose a reason for hiding this comment

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

@miles-grant-ibigroup This is confusing me. If the value is ignored, why can't the end point above be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it, we can't make a call to .../artifact/blah.json which means we have to call it as .../artifact which means the downloaded file is downloaded as artifact which is no good.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that makes sense. Can you update the comment with this explanation please and perhaps call it :expectedFileName instead of :ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

br648
br648 approved these changes Jul 16, 2025
View reviewed changes
Copy link
Contributor

@br648 br648 left a comment

Choose a reason for hiding this comment

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

Just the one item to address, but approving.

src/main/java/com/conveyal/datatools/manager/controllers/api/DeploymentController.java Outdated
@@ -602,6 +607,8 @@ public static void register (String apiPrefix) {
options(apiPrefix + "secure/deployments", (q, s) -> "");
get(apiPrefix + "secure/deployments/:id/download", DeploymentController::downloadDeployment);
get(apiPrefix + "secure/deployments/:id/artifact", DeploymentController::downloadBuildArtifact);
// This path allows the downloaded artifact file to be named anything
get(apiPrefix + "secure/deployments/:id/artifact/:ignored", DeploymentController::downloadBuildArtifact);
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that makes sense. Can you update the comment with this explanation please and perhaps call it :expectedFileName instead of :ignored?

@br648 br648 assigned miles-grant-ibigroup and unassigned br648 Jul 16, 2025
binh-dam-ibigroup
binh-dam-ibigroup requested changes Jul 23, 2025
View reviewed changes
Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

The link alone is not going to work as written. I think it needs to go to datatools UI first where the download can be triggered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@binh-dam-ibigroup binh-dam-ibigroup binh-dam-ibigroup requested changes

@br648 br648 br648 approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@miles-grant-ibigroup miles-grant-ibigroup

Labels
enhancement WIP Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@miles-grant-ibigroup @br648 @binh-dam-ibigroup @JymDyerIBI