Skip to content
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(schedule)!: show schedule errors and nest object #795

Merged
merged 9 commits into from
May 10, 2024

Conversation

wsan3
Copy link
Contributor

@wsan3 wsan3 commented Apr 26, 2024

Description

Whenever a user has a YAML bug in their pipeline, or any other build-breaking error that short-circuits pipeline execution, they can usually find these error in the Audit tab, where we populate the hook with the error message.

This is currently not available for schedules. Instead, the error is just logged.

Desired View

Screenshot 2024-04-26 at 10 37 57 AM

Related

go-vela/community#872

@wsan3 wsan3 added the feature Indicates a new feature label Apr 26, 2024
@wsan3 wsan3 self-assigned this Apr 26, 2024
@wsan3 wsan3 requested a review from a team as a code owner April 26, 2024 15:49
@plyr4
Copy link
Contributor

plyr4 commented Apr 29, 2024

whats the plan for adding the other changes made in go-vela/server#1108 like nested repos?
will those be in their own PR or attached to this one?

@wsan3
Copy link
Contributor Author

wsan3 commented May 1, 2024

whats the plan for adding the other changes made in go-vela/server#1108 like nested repos? will those be in their own PR or attached to this one?

I'll attach the nested Schedule and types migration changes to this PR

@wsan3 wsan3 marked this pull request as draft May 1, 2024 14:21
@wsan3 wsan3 marked this pull request as ready for review May 2, 2024 18:36

msgRow =
tr [ class "error-data", Util.testAttribute "schedules-error" ]
[ td [ attribute "colspan" "6" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

is this "6" supposed to match the number of columns?
if so maybe we can make the value dynamic depending on the number of headers?

Suggested change
[ td [ attribute "colspan" "6" ]
[ td [ attribute "colspan" (String.fromInt <| List.length tableHeaders) ]

Copy link
Contributor

@plyr4 plyr4 May 8, 2024

Choose a reason for hiding this comment

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

im just nitpicking. i realize now that its hardcoded in some places, so whatever.

but i was thinking about this spot when i wrote that:
https://github.com/go-vela/ui/blob/main/src/elm/Components/Table.elm#L131

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took that snippet from viewHookError in src/elm/Pages/Org_/Repo_/Hooks.elm

Copy link
Contributor

Choose a reason for hiding this comment

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

sweet, figured 👍

@wsan3 wsan3 requested a review from plyr4 May 8, 2024 14:01
@wsan3 wsan3 changed the title feat(schedule): show schedule errors feat(schedule)!: show schedule errors and nest object May 8, 2024
plyr4
plyr4 previously approved these changes May 8, 2024
Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

looks good overall. nice job. only thing missing that i would like to see is a (cypress) test to make sure that error is shown when returned from a schedule call.

@wsan3
Copy link
Contributor Author

wsan3 commented May 8, 2024

looks good overall. nice job. only thing missing that i would like to see is a (cypress) test to make sure that error is shown when returned from a schedule call.

Good suggestion. I'll create one.

wass3r
wass3r previously approved these changes May 10, 2024
Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

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

thanks

@wsan3 wsan3 merged commit b316a3a into main May 10, 2024
13 checks passed
@wsan3 wsan3 deleted the feat/show-schedule-errors branch May 10, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants