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

ci(schema): check for OpenAPI and GraphQL schema changes #334

Merged
merged 50 commits into from
Mar 20, 2024

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Mar 19, 2024

Welcome to Cryostat3! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Related to #333

Description of the change:

Checks in the OpenAPI definition to schemas/openapi.yaml, transformed with yq so that the keys are sorted and therefore can be compared across revisions without worrying about exact key ordering. The GraphQL schema is also checked in to schemas/schema.graphql, but this is commented out since it depends on #294 .

Then, adds a CI action on /build_test that runs on PR to regenerate the schema(s) within the CI runner and check if the PR schema differs from the main schema. A bot comment indicates change/no-change for each schema, including the diff if there is a change found.

Sample run: https://github.com/andrewazores/cryostat3/actions/runs/8361776180

image

The GraphQL schema change is expected, this patch was applied for testing: andrewazores#1

diff --git a/src/main/java/io/cryostat/graphql/ArchivedRecordings.java b/src/main/java/io/cryostat/graphql/ArchivedRecordings.java
index afcf36f..d6fcbbf 100644
--- a/src/main/java/io/cryostat/graphql/ArchivedRecordings.java
+++ b/src/main/java/io/cryostat/graphql/ArchivedRecordings.java
@@ -39,6 +39,11 @@ public class ArchivedRecordings {
 
     @Inject RecordingHelper recordingHelper;
 
+    @Query("hello")
+    public String hello() {
+        return "hello there";
+    }
+
     @Blocking
     @Query("archivedRecordings")
     public TargetNodes.ArchivedRecordings listArchivedRecordings(ArchivedRecordingsFilter filter) {

Motivation for the change:

This is meant to help developers and maintainers notice when they have made changes to the exposed interfaces, to help prevent inadvertent changes which may break compatibility, as well as to highlight intentional breakages or new features to help inform the next release's semantic versioning name.

@github-actions github-actions bot added needs-triage Needs thorough attention from code reviewers dependent labels Mar 19, 2024
Copy link

This PR/issue depends on:

@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Mar 19, 2024
@andrewazores andrewazores marked this pull request as ready for review March 20, 2024 15:58
@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 3/20/2024, 12:09:31 PM. View Actions Run.

Copy link

CI build and push: All tests pass ✅ (JDK21)
https://github.com/cryostatio/cryostat3/actions/runs/8362172946

Copy link

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8362172946

@ebaron
Copy link
Member

ebaron commented Mar 20, 2024

It seems like it's possible to do the sorting with an OpenAPI filter in Quarkus: quarkusio/quarkus#28509

That looks like more work than simply installing and using yq though.

@andrewazores andrewazores merged commit 465ff68 into cryostatio:main Mar 20, 2024
7 of 8 checks passed
@andrewazores andrewazores deleted the schema-tracking branch March 20, 2024 17:25
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.

2 participants