Skip to content

Improve error handling in post-cdash-status - #4

Merged
vicentebolea merged 4 commits into
Kitware:masterfrom
hainest:thaines/update_error_handling
Sep 1, 2026
Merged

Improve error handling in post-cdash-status#4
vicentebolea merged 4 commits into
Kitware:masterfrom
hainest:thaines/update_error_handling

Conversation

@hainest

@hainest hainest commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

This also adds authentication to the status query to match the rate-limit of the POST. I'm not sure if it's strictly necessary, but I found it useful when testing.

Comment thread post-cdash-status
Comment thread post-cdash-status Outdated
Comment thread post-cdash-status Outdated
Comment thread post-cdash-status
@hainest

hainest commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@vicentebolea I'd forgotten about this. I think it's ready for final review.

@vicentebolea

Copy link
Copy Markdown
Member

@hainest looks good, this might be out of the scope of this PR, it will be nice to add a unit test that checks the status of a a known commit in cdash project. Feel free to do it here or in a followup PR

@hainest

hainest commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@hainest looks good, this might be out of the scope of this PR, it will be nice to add a unit test that checks the status of a a known commit in cdash project. Feel free to do it here or in a followup PR

Is that possible to do? I thought the dashboard history is cleared periodically.

@vicentebolea

Copy link
Copy Markdown
Member

@hainest looks good, this might be out of the scope of this PR, it will be nice to add a unit test that checks the status of a a known commit in cdash project. Feel free to do it here or in a followup PR

Is that possible to do? I thought the dashboard history is cleared periodically.

we could inquire a recent commit of a project such a cmake.

@hainest

hainest commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@hainest looks good, this might be out of the scope of this PR, it will be nice to add a unit test that checks the status of a a known commit in cdash project. Feel free to do it here or in a followup PR

Is that possible to do? I thought the dashboard history is cleared periodically.

we could inquire a recent commit of a project such a cmake.

Sounds good. I'll make a separate PR for that.

@hainest

hainest commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@vicentebolea It looks like I'm not allowed to merge. Could you do it for me?

@vicentebolea
vicentebolea merged commit 392e931 into Kitware:master Sep 1, 2026
2 checks passed
@vicentebolea

Copy link
Copy Markdown
Member

@vicentebolea

Copy link
Copy Markdown
Member

@hainest

hainest commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@vicentebolea It's failing because the status is associated with the HEAD of the PR (fb36010), but it's testing the HEAD of master which is 392e931. That test will only pass on pushes for single-commit PRs that aren't rebase-and-merge or squash-and-merge.

@hainest
hainest deleted the thaines/update_error_handling branch September 1, 2026 20:37
@vicentebolea

Copy link
Copy Markdown
Member

@vicentebolea It's failing because the status is associated with the HEAD of the PR (fb36010), but it's testing the HEAD of master which is 392e931. That test will only pass on pushes for single-commit PRs that aren't rebase-and-merge or squash-and-merge.

Good catch. lets only allow merge commit merges to avoid this.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants