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

fix: refactor workflow_job.queued event to use the WorkflowJob common shema #657

Merged

Conversation

Helcaraxan
Copy link
Contributor

@Helcaraxan Helcaraxan commented Jul 5, 2022

My personal code generation tool led me to notice a missing refactor for the queued variant of the workflow_job event. Am refactoring it here along the same lines as what is used for other variants of the same event.

This does require the relaxation of the minItems constraints on the steps field in the refactored Workflow Job spec and hence results in a slight alteration of the validation semantics of the other event variants, without it being a breaking change.

If someone could point out a way to forgo this constraint relaxation I am all ears but I did try to add an override for steps alongside the status one within the queued spec but this resulted in test failures.

Example, adding the following in queued.schema.json:

        {
          "type": "object",
          "required": ["status"],
          "properties": {
            "status": { "type": "string", "enum": ["queued"] },
            "steps": {
              "type": "array",
              "items": { "$ref": "common/workflow-step.schema.json" }
            }
          },
          "tsAdditionalProperties": false
        }

results on npm run test in:

✅ Payload 'workflow_job/completed.failure.with-organization.payload.json' matches schema
✅ Payload 'workflow_job/completed.success.with-organization.payload.json' matches schema
✅ Payload 'workflow_job/in_progress.payload.json' matches schema
❌ Payload 'workflow_job/queued.payload.json' does not match schema
[
  {
    instancePath: '/action',
    schemaPath: '#/properties/action/enum',
    keyword: 'enum',
    params: { allowedValues: [ 'completed', 'in_progress' ] },
    message: 'must be equal to one of the allowed values'
  },
  {
    instancePath: '/workflow_job/steps',
    schemaPath: '#/properties/steps/minItems',
    keyword: 'minItems',
    params: { limit: 1 },
    message: 'must NOT have fewer than 1 items'
  },
  {
    instancePath: '',
    schemaPath: '#/oneOf',
    keyword: 'oneOf',
    params: { passingSchemas: null },
    message: 'must match exactly one schema in oneOf'
  }
]

@ghost ghost added this to Inbox in JS Jul 5, 2022
@wolfy1339
Copy link
Member

This would require some fancier work to get the validation to work.

https://json-schema.org/understanding-json-schema/reference/conditionals.html#if-then-else

You can't really extend schemas in the way you would expect in object-oriented programming. Each schema defined in an allOf must pass independently of one another, which is why the minItems needs to be relaxed

@Helcaraxan
Copy link
Contributor Author

@wolfy1339 thank you very much for the insight. I understand it better now.

Despite the relaxation is this PR something that you would entertain as a refactor or would it be better to close and keep things as they are right now?

@wolfy1339
Copy link
Member

I think it's fine to relax the minItems

@wolfy1339 wolfy1339 changed the title feat: Refactor workflow_job queued event fix: refactor workflow_job.queued event to use the WorkflowJob common shema Jul 5, 2022
@wolfy1339 wolfy1339 added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Jul 5, 2022
@ghost ghost moved this from Inbox to Maintenance in JS Jul 5, 2022
@wolfy1339 wolfy1339 enabled auto-merge (squash) July 5, 2022 20:14
@wolfy1339 wolfy1339 merged commit 9c2d5f6 into octokit:master Jul 5, 2022
JS automation moved this from Maintenance to Done Jul 5, 2022
@Helcaraxan Helcaraxan deleted the feat/simplify-workflow-job-queued branch July 5, 2022 20:15
@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 6.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
No open projects
JS
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants