-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
feat: add new BitbucketCloudEntityProvider and common/shared client lib #11345
feat: add new BitbucketCloudEntityProvider and common/shared client lib #11345
Conversation
Changed Packages
|
f3c3cd9
to
5341e17
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.
This is huge work, thank you. I hope the comments don't deter.
plugins/catalog-backend-module-bitbucket-cloud/src/BitbucketCloudEntityProvider.test.ts
Outdated
Show resolved
Hide resolved
plugins/catalog-backend-module-bitbucket-cloud/src/BitbucketCloudEntityProvider.ts
Outdated
Show resolved
Hide resolved
5341e17
to
0ccf521
Compare
a4356e6
to
8f1bc3b
Compare
8f1bc3b
to
1409458
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.
As already mentioned, great work(as always) Patrick!
The one thing that really complicates this PR is the client generation making the PR gigantic with all the schemas and generated API report.
I'm slightly worried that this ties us too close to the generated code and client. In most cases we try hard not tying ourselves too hard to for other libraries to be able to switch out the client when a new one pops up which is fairly common in javascript land.
Secondly we have very little tooling and common knowledge with schema generation in the repository making this a big risk of technical depth if you decide to work on other things in the future. Is there alternatives to the generation approach proposed in this PR?
I hope you don't take take this as negativity as introducing this method of generation is a big deal for the repository and tooling so it require careful consideration if we go down this path :)
@jhaals all fair points! Let's start with alternatives:
Yes, you are right. It got big due to it being the first time and including all models. Only including the models which are actually used by the client implementation would reduce it considerably, though. Maybe a bit harder to add additional endpoints (without existing models).
At the end, we control the client and how it looks like by the templates used to generate them. The client lib is inside the backstage project, too. It is easier to change compared to external client libraries.
Picking up the generator didn't took me long. Code generators for OpenAPI schema (client and/or server) are not that uncommon. Still, it takes a moment to get into this, of course.
External libraries are likely more feature complete. Extensibility depends on the implementation. Or, we could handwrite all models and APIs. Only those we need at the moment. Manually creating the models vs generating is more error prone though. The generation is just a means to stay in sync and correct. It should help reduce maintenance efforts. As you saw, I was doing some improvements to the API schema (has a few bugs; other parts make it easier for generating the right output).
Fully agree and open for discussion and open for changing the approach, too. |
1409458
to
82aa8d1
Compare
42be7a6
to
d7de3e6
Compare
3b88954
to
02a291a
Compare
a8f6f5d
to
f96245e
Compare
E2E test/cypress is timing out a lot. Trying to retrigger the build by reuploading the last commit |
f96245e
to
5ae5fc9
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.
Awesome stuff overall 😁 ❤️
Brought up some concerns about the code generation, but I think we should be able to figure out a way to ship this.
I'm feeling that if we do end up stuck in the future where we can't update the model, there is always the option of to drop the generation and continue with hand crafted models, especially if needed for a temporary fix.
Btw, a middle-ground that I want to hear your thoughts on is: what do you think about using the code generation only as a tool for authoring code? So rather using the generated model directly, we copy over the subset that we have the need for in the project right now? That'd reduce the API surface a lot. We could still keep the generated types and modifications so that it's as close to a simple copy-paste as possible, even in source control for easy diffing. Then over time if we eventually feel we just want the entire thing we could start using the generated code directly instead?
Not gonna block this on the failing cypress tests btw, but will keep an eye on that.
Sure, we can do that. Manually is easy, automatic a bit more tricky. ..but yeah.. I guess it makes sense. I was a bit in-between as generating the full model makes adding new endpoints a little easier. But smaller API surface is great, too. |
5ae5fc9
to
6e1c13a
Compare
Add `bitbucket-cloud-common` (`@backstage/plugin-bitbucket-cloud-common`) with a new client for Bitbucket Cloud. The lib contains auto-generated models which got generated using `@openapitools/openapi-generator-cli`. Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
…itbucket` Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
6e1c13a
to
54f5616
Compare
@Rugvip I took care of all points including reducing the models to have the minimal API surface possible. It is automated ( As this is my first use of ts-morph, let me know if you see any potential improvements. :) |
d9e2c7a
to
caab993
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.
Thank you! 🎉
Reduced model does look nicer, and I'm able to verify that it's stable too, at least in my environment. Feeling it's time to
Tiny nit to fix but not a blocker, just giving @backstage/maintainers some extra time to have a look again 😁
Add a new entity provider `BitbucketCloudEntityProvider` as a new plugin `@backstage/plugin-catalog-backend-module-bitbucket-cloud`. The new plugin utilizes `@backstage/plugin-bitbucket-cloud-common` and it fully independent of `@backstage/plugin-catalog-backend-module-bitbucket` which provides a catalog processors supporting Bitbucket Cloud and Bitbucket Server. Relates-to: backstage#9923 Relates-to: backstage#10183 Signed-off-by: Patrick Jungermann <Patrick.Jungermann@gmail.com>
caab993
to
dfc4efc
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.
Yep - ok. Let's get this in 🚢
In the |
@diegomarangoni Unfortunately, at the moment only default rules apply as these rules need to be added to the Location entities which is managed by the static config entity provider reading the locations from the app-config.yaml. I mentioned this case at another PR, too I plan to add a feature for it, but it still does not exist. And there is no issue yet. |
@diegomarangoni I created issue #12880 as follow up related to the "allow" rule |
Hey, I just made a Pull Request!
✔️ Checklist
Signed-off-by
line in the message. (more info)