-
Notifications
You must be signed in to change notification settings - Fork 5
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: Backstage API entity compatibility #182
Conversation
a71efac
to
edd3b2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though I have some questions in the diff.
internal/backstage/backstage.go
Outdated
if r >= 'a' && r <= 'z' { | ||
return r | ||
} | ||
return '-' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do really want to replace everything else with -
? Maybe just drop them? Or replace them with -
if they have an alphanum to either side?
I suppose the latter approach would require regex processing which would be Bad(TM). But maybe just replacing spaces and ~ with the -, and drop other characters? The example in the test with a ton of trailing dashes just looks weird to me, but I'm willing to be told I'm overthinking this and that sort of case isn't likely to actually come up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree -- one of my unit tests already kind of makes this point as well, thanks for raising this!
- Registry_2021-06-07_experimental | ||
- Registry_2021-06-13_beta | ||
- Registry_2021-06-13_experimental | ||
- Registry_2021-08-20_beta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be e.g. Registry-2021-08-20-beta
? Aren't underscores a replaced character? Or am I now mixing up which parts are changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to replace invalid characters in the API name prefix (derived from .info.title in the OpenAPI spec) with dashes, then join that with the version date and stability using underscores. Following your above suggestion, I think I might replace only spaces with dashes, and drop all other invalid chars.
This has a nice property of being able to split the name on '_' into three parts (name, version date, stability). If I use dashes to join the name and version, parsing is still possible, but gets a little more complicated.
I'm not quite sure what I'm going to be able to pull out of Backstage in various integrations to do reporting, so I'm leaving room for some optionality here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, so this is the final product of the join--I had missed that detail. And yes, I can see the utility of having a uniform separator that also scans pretty well in titles.
edd3b2a
to
56a9fb5
Compare
A few fixes needed in order to generate API entities that Backstage will accept: - API metadata name must match ([A-Z][a-z][0-9]_-)+ - Annotations cannot nest, only one level supported. Deduplicate providesApis. Deterministic order of generated API entities. Added a more readable, user-friendly title. Version information is placed under labels. Tags are also added, derived from versioning fields.
56a9fb5
to
e8f6b54
Compare
A few fixes needed in order to generate API entities that Backstage will
accept:
Added a more readable, user-friendly title.
Version information is placed under labels.
Tags are also added, derived from versioning fields.