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

Expose build errors and warnings via GraphQL #2182

Merged
merged 3 commits into from
May 9, 2024

Conversation

williamjallen
Copy link
Collaborator

This PR continues our ongoing effort to expose most of the database schema via GraphQL.

@williamjallen williamjallen added this to the v3.5 milestone Apr 30, 2024
@williamjallen williamjallen marked this pull request as draft April 30, 2024 19:20
@williamjallen
Copy link
Collaborator Author

Paused until #2183 is merged.

@williamjallen williamjallen marked this pull request as ready for review May 2, 2024 14:42
@williamjallen williamjallen requested a review from josephsnyder May 2, 2024 14:42
josephsnyder
josephsnyder previously approved these changes May 6, 2024
Copy link
Member

@josephsnyder josephsnyder left a comment

Choose a reason for hiding this comment

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

The GraphQL changes in the Explorer look good. After doing a little exploring with the CTest data, I was able to find a place where I thought things didn't align but the GraphQL return matches the data found in the build table
image

@williamjallen
Copy link
Collaborator Author

After discussion, it looks like there are some issues that need to be resolved before this can be merged. The buildWarningsCount being wrong isn't terribly surprising given that this information was not generated by our regular submission process, but the fact that no warnings were selected is concerning.

@williamjallen williamjallen marked this pull request as draft May 6, 2024 21:29
@williamjallen williamjallen dismissed josephsnyder’s stale review May 6, 2024 21:31

Needs further work. Current functionality works as intended, but intended purpose was not the best long-term design decision.

@williamjallen williamjallen marked this pull request as ready for review May 9, 2024 12:32
@williamjallen williamjallen requested a review from josephsnyder May 9, 2024 12:32
@williamjallen
Copy link
Collaborator Author

@josephsnyder I think this is ready for a re-review.

The issue you observed is the result of my confusion about the fact that CDash stores two different types of warnings or errors. Adding the other type of warnings/errors to the API will be a significant undertaking, so I think it should be done in another PR (or several). This PR specifically adds the "basic" errors and warnings stored in the builderror table. Functionally, it should be equivalent to the behavior during your last review.

To reduce confusion about the fact that "build errors" can actually be warnings, I have created the term "build alert" to refer to the collection of both errors and warnings, with "build errors" and "build warnings" being more specific types of build alerts. Furthermore, I have created the terms "basic" and "rich" alerts to refer to the contents of the builderror and buildfailure tables respectively. To get all of the errors or warnings for a given build, one must query both the basic and (yet-to-be-implemented) rich errors or warnings. I'm open to suggestions regarding better terminology for any of these.

It would be good for @zackgalbreath to review this as well, particularly the documentation and terminology changes.

Copy link
Member

@josephsnyder josephsnyder left a comment

Choose a reason for hiding this comment

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

I can see the BasicBuildAlert doc type and it does nicely translate to a query where you can ask for a build for it's basicWarnings and BasicErrors with the context, line, and all the other expected fields. It LGTM but I'll hold off on adding it to the merge queue to give Zack a chance to chime in

@williamjallen williamjallen added this pull request to the merge queue May 9, 2024
Merged via the queue into Kitware:master with commit 4f4d850 May 9, 2024
6 checks passed
@williamjallen williamjallen deleted the builderror-graphql branch May 9, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants