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

add catalog-info for backstage #137

Merged
merged 12 commits into from
May 12, 2023

Conversation

rosesyrett
Copy link
Contributor

related to #113

@rosesyrett rosesyrett changed the title add catalog-info for backstage WIP: add catalog-info for backstage Apr 21, 2023
@rosesyrett
Copy link
Contributor Author

rosesyrett commented Apr 21, 2023

This should not be merged until #135 is merged. This PR sits on top of it, which is why it has so many changes.

@keithralphs
Copy link
Contributor

AsyncAPI yaml file has now been added at docs/user/reference/asyncapi.yaml, so please add a reference to this to the cataloginfo file. Also please put your openAPI file in the same folder and update the reference to it accordingly.

@DiamondJoseph
Copy link
Contributor

Be nice to have this validator for the catalog-info, but I'm happy to add it once this is over the line

@keithralphs
Copy link
Contributor

keithralphs commented May 5, 2023

Also be good to add

annotations:
    github.com/project-slug: DiamondLightSouce/blueapi

to the component>metadata section so that the CI and other stuff is visible in backstage

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #137 (9c035fb) into main (76a4e02) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
+ Coverage   87.39%   87.53%   +0.13%     
==========================================
  Files          40       41       +1     
  Lines        1079     1091      +12     
==========================================
+ Hits          943      955      +12     
  Misses        136      136              
Impacted Files Coverage Δ
src/blueapi/service/openapi.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rosesyrett rosesyrett changed the title WIP: add catalog-info for backstage add catalog-info for backstage May 11, 2023
@rosesyrett rosesyrett requested a review from DiamondJoseph May 11, 2023 13:18
providesApis:
- message-topics
- restapi
- blueskydocument-to-scanmessage
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the ugliness of this (since it's not actually emitting ScanMessages, and ScanMessages are defined elsewhere, and this whole provided API is defined elsewhere) is reason enough to say: now is the time to do #167 and fold it into this change.

Copy link
Contributor

@keithralphs keithralphs May 11, 2023

Choose a reason for hiding this comment

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

I would prefer to address that in a separate change that also includes updating the AsyncAPI definition to reflect that some of it's functionality is now repaced by the REST API. That will allow us to close this ticket and its associated Jira one and schedule the cleanup activity for next sprint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be defined in a workflow instead of in the source code? Is there already a workflow to handle autogeneration of OpenAPI Schemas?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would mean we couldn't update the OpenAPI definition without submitting the change to GitHub, which sounds wrong to me; I think it should exist in the repo before GitHub is involved to facilitate testing of the API.

docs/user/reference/openapi.json Show resolved Hide resolved
@@ -0,0 +1,5 @@
OpenAPI Specification
Copy link
Contributor

Choose a reason for hiding this comment

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

You can actually get sphinx to render the spec in a swagger-like way with a plugin, see example in scanspec: https://github.com/dls-controls/scanspec/blob/master/docs/reference/rest_api.rst

Also, I think we should rethink calling the docs "openapi.rst" and "asyncapi.rst", as that only defines the type, not the purpose, of the API they are documenting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. I do think however that renaming the docs is maybe a separate issue that can tie into #181?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gonna have a look at that plugin though.

@rosesyrett rosesyrett merged commit 6230cc2 into DiamondLightSource:main May 12, 2023
@rosesyrett rosesyrett deleted the backstage branch May 12, 2023 09:20
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