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

Openapi #1

Merged
merged 1 commit into from
Feb 3, 2022
Merged

Openapi #1

merged 1 commit into from
Feb 3, 2022

Conversation

jasmingacic
Copy link
Contributor

This PR...

Changes

Fixes

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
get:
parameters:
- in: path
name: id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be 'testid' rather than 'id' to match what you have above (ie /tests/{testid}/results/{id})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since there is only a single parameter here it was self explanatory. But anything will work

# type: array
# items:
# $ref: "#/components/schemas/Problem"
/tests/{testid}/results/{id}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to explicitly name our 'id' everywhere or not? We are doing both here - using 'testid' but not using 'resultid'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"testid" is for the test and the other one is result id. It can work either way so it isn't too relevant. Ideally both should be "id" but then in the code we would have a mess.

Anyways whichever way we pick will produce the same result.

Copy link
Member

Choose a reason for hiding this comment

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

use testId and resultId

api/openapi.yaml Outdated
duration:
type: string
format: duration
nrOfSPans:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Picky comment... SPans should be Span. Really picky - should this be named nrOfSpans or numOfSpans - i have not seen nr as an abbreviation for number often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I will adjust it.

@jasmingacic jasmingacic merged commit f084ff9 into main Feb 3, 2022
duration:
type: string
format: duration
numOfSPans:
Copy link
Member

Choose a reason for hiding this comment

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

name seems wrong - numberOfSpans? spanCount?

# description: object name
Test:
type: object
properties:
Copy link
Member

Choose a reason for hiding this comment

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

missing a read-only id property?

Result:
type: object
properties:
successful:
Copy link
Member

Choose a reason for hiding this comment

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

missing a read-only id property?

items:
$ref: "#/components/schemas/Assertion"
description: Definition of assertions that are going to be made
repeats:
Copy link
Member

Choose a reason for hiding this comment

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

what is this?


components:
schemas:
# ObjectRef:
Copy link
Member

@olensmar olensmar Feb 3, 2022

Choose a reason for hiding this comment

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

remove commented sections?

# description: object name
Test:
type: object
properties:
Copy link
Member

Choose a reason for hiding this comment

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

should have required sections for each object - so the client knows which values to rely on

schoren pushed a commit that referenced this pull request Mar 8, 2024
#1)

chore: update automated deployments on stage and add sync instructions
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.

4 participants