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 event support for the Catalog Plugin GitLab Discovery Entity providers #23373

Merged

Conversation

elaine-mattos
Copy link
Contributor

@elaine-mattos elaine-mattos commented Mar 3, 2024

Hey, I just made a Pull Request!

Add events support for the GitLab Entity Provider modules.
Refs: RFC: Event Subscription Mechanism for Cross-Plugin Module Integration in the New Backend System #22393 and 🚀 Feature: Add Events Support for GitlabDiscoveryEntityProvider and GitlabOrgDiscoveryEntityProvider #21553

✔️ TODO List

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@github-actions github-actions bot added the area:catalog Related to the Catalog Project Area label Mar 3, 2024
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Mar 3, 2024

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-catalog-backend-module-gitlab-org plugins/catalog-backend-module-gitlab-org patch v0.0.0
@backstage/plugin-catalog-backend-module-gitlab plugins/catalog-backend-module-gitlab patch v0.3.15-next.1

Copy link
Contributor

github-actions bot commented Mar 3, 2024

Uffizzi Cluster pr-23373 was deleted.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 3, 2024
@elaine-mattos elaine-mattos marked this pull request as ready for review March 3, 2024 17:36
Copy link
Member

@vinzscam vinzscam left a comment

Choose a reason for hiding this comment

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

amazing work 👏
Just added a tiny comment about the changesets, otherwise ✨

@elaine-mattos elaine-mattos force-pushed the feat/add-event-support-gitlab-provider branch from 4db44fb to 580dcf8 Compare March 11, 2024 21:15
@elaine-mattos elaine-mattos reopened this Mar 16, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Mar 16, 2024
@elaine-mattos elaine-mattos marked this pull request as draft March 16, 2024 11:26
@elaine-mattos elaine-mattos marked this pull request as ready for review March 16, 2024 17:41
@elaine-mattos
Copy link
Contributor Author

Hi @vinzscam !

I had some issues with my branch and had to recreate it (I know, I'm such a git newbie! :D ).
I did incorporate your suggestions/remarks. Could you please take a look at them?
Also, the pipeline is throwing a formatting error on a file I did not touch (beps/0004-scaffolder-task-idempotency/README.md). Should I do anything about it?

thanks again in advance!!! :)

@freben
Copy link
Member

freben commented Mar 19, 2024

Yeah that prettier error has been fixed since then. Rebasing on a fresh master gets rid of the problem. Something like this

git checkout master
git pull
git checkout feat/add-event-support-gitlab-provider
git pull
git rebase master

@elaine-mattos elaine-mattos force-pushed the feat/add-event-support-gitlab-provider branch from 093d668 to 24b2bbb Compare March 20, 2024 21:17
@elaine-mattos elaine-mattos marked this pull request as draft March 20, 2024 21:18
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 20, 2024
@elaine-mattos elaine-mattos marked this pull request as ready for review March 20, 2024 22:27
@elaine-mattos
Copy link
Contributor Author

Hi guys,

the suggested changes have been made. Could you please take a look?
I appreciate the feedback :)

@elaine-mattos elaine-mattos marked this pull request as draft April 15, 2024 17:01
@elaine-mattos elaine-mattos force-pushed the feat/add-event-support-gitlab-provider branch from 2bad1cc to 07eb276 Compare April 15, 2024 17:04
@elaine-mattos elaine-mattos marked this pull request as ready for review April 15, 2024 20:03
@elaine-mattos
Copy link
Contributor Author

Hi @elaine-mattos, can you please add details about the fact that you need to define the schedule in your provider config when using the new backend system? This is missing in the current docs and I don't see it here. Thanks!

Hi @awanlin ! I've changed the docu a little bit. Could you please review? :)

@elaine-mattos
Copy link
Contributor Author

also very sorry but a rebase is needed now

Hi @freben , done! :)

Copy link
Collaborator

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Thanks for adding in the schedule details to the docs @elaine-mattos, I left two comments but they are the same. As I haven't reviewed the code just the docs I'm only leaving this review as a comment

groupPattern: '[\s\S]*' # Optional. Filters found groups based on provided pattern. Defaults to `[\s\S]*`, which means to not filter anything
schedule: # optional; same options as in TaskScheduleDefinition
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this true in all cases that it's optional? If I'm using the New Backend System would that not then be required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @awanlin !

I changed this line. Could you please check? :)

docs/integrations/gitlab/discovery.md Outdated Show resolved Hide resolved
elaine-mattos and others added 15 commits April 18, 2024 08:54
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: elaine-mattos <elaine.mattos@gmail.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
@elaine-mattos
Copy link
Contributor Author

Hi @vinzscam, @freben , @awanlin ,

is there anything else I can do to get this one merged?
Please let me know if there is anything missing! :)

Copy link
Member

@freben freben left a comment

Choose a reason for hiding this comment

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

OK, phew! I left some comments that can be deferred to a follow-up PR some time down the line. Do check them out. But for now, :shipit:

As this provider is not one of the default providers, you will first need to install the Gitlab provider plugin:

```bash
# From your Backstage root directory
yarn --cwd packages/backend add @backstage/plugin-catalog-backend-module-gitlab
yarn --cwd packages/backend add @backstage/plugin-catalog-backend-module-gitlab @backstage/plugin-catalog-backend-module-gitlab-org
Copy link
Member

Choose a reason for hiding this comment

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

really both?

coreServices,
createBackendModule,
} from '@backstage/backend-plugin-api';
import { GitlabOrgDiscoveryEntityProvider } from '@backstage/plugin-catalog-backend-module-gitlab';
Copy link
Member

Choose a reason for hiding this comment

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

I do agree that the implementation should really be moved over to the package where it belongs, and deprecated in the old place. If in a follow-up PR - fine. But should probably be done soonish :)

"@backstage/plugin-catalog-node": "workspace:^",
"@backstage/plugin-events-node": "workspace:^",
"@gitbeaker/rest": "^40.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"@gitbeaker/rest": "^40.0.3",
"@gitbeaker/rest": "^39.25.0",

if that's okay, to align with the other deps in the repo - this should clean up the yarn.lock diff

]
}
},
"version": "0.3.15-next.1"
Copy link
Member

Choose a reason for hiding this comment

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

not sure what happened with the field order sorting here :) version is usually way up at the top etc, but we can sort (hah!) that out some other time

* @public
*/
// <<< EventSupportChange: implemented EventSubscriber interface
Copy link
Member

Choose a reason for hiding this comment

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

remove

@@ -96,7 +133,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider {
const integration = integrations.byHost(providerConfig.host);

if (!providerConfig.orgEnabled) {
return;
throw new Error(`Org not enabled for ${providerConfig.id}.`);
Copy link
Member

Choose a reason for hiding this comment

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

Is this really a safe change?

@freben freben merged commit 3778678 into backstage:master Apr 22, 2024
46 checks passed
Copy link
Contributor

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.27.0 release, scheduled for Tue, 14 May 2024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:catalog Related to the Catalog Project Area documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants