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

chore(scaffolder-backend-module-gitlab): Upgrade to gitbeaker ^41.2.0 #27581

Conversation

mpsanchis
Copy link
Contributor

Hey, I just made a Pull Request!

Upgrade gitbeaker to ^41.2.0 (major version) in scaffolder-backend-module-gitlab.

First split of #27360 as requested.

✔️ 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:scaffolder Everything and all things related to the scaffolder project area label Nov 11, 2024
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Nov 11, 2024

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-scaffolder-backend-module-gitlab plugins/scaffolder-backend-module-gitlab minor v0.6.1-next.3

@backstage-goalie
Copy link
Contributor

Thanks for the contribution!
All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

@mpsanchis mpsanchis force-pushed the mpsanchis/upgrade-gitbeaker-gitlab-scaffolder branch from 050efe6 to ecef3cf Compare November 11, 2024 14:44
@CptnFizzbin
Copy link
Contributor

do you know if this update would fix the error messages from GitLab (as I reported in #27600)

For example when trying to create a branch via the API, if there are branch name rules configured, the error message is added to a description field instead of the error's message

@mpsanchis
Copy link
Contributor Author

Hi @CptnFizzbin
I haven't tried it, I'm not sure. In fact this PR is a "prerequisite" for me, because I want to add a feature that only newer versions of Gitbeaker support. I have done the bare minimum to do the upgrade (ideally I would have only modified package.jsons). Feel free to give it a try and let me know 😄

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.

Alright, looking good, just minor questions

| 'write_registry'
| 'read_package_registry'
| 'write_package_registry'
)[];
Copy link
Member

Choose a reason for hiding this comment

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

Hm. Is this a good thing being the specific strings? Is this truly the only possible scopes now and forever? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Up to you guys :) Having it as string allows "unwanted" behaviour for now, but of course the user will quickly realize.

I'm happy to leave it as a string. Should I cast it as DeployTokenScope in createGitlabProjectDeployTokenAction when calling api.DeployTokens.create?

Copy link
Contributor

Choose a reason for hiding this comment

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

might be good to list out the known good ones, and have | string tagged onto the end. Would allow intellisense to suggest these values, while also providing an escape hatch (and type hints) that there could be more options in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Yep that's a good middle ground. Will still need a cast internally but is useful to the end user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give me a hand on this, guys? TS considers some types as "subtypes" of others. For instance:

let a: string | "read_repository" | "read_registry" | "write_registry" | "read_package_registry" | "write_package_registry";

is equivalent (in VSCode suggestions) to:

let a: string;

With zod, these types would be generated with:

scopes: z.array(
  z.union([
    z.enum([
      'read_repository',
      'read_registry',
      'write_registry',
      'read_package_registry',
      'write_package_registry',
    ]),
    z.string()
  ]),
  { description: 'Scopes' },

But I get no benefit with intellisense, and the API report will declare string[].

I'm good with reverting it to string[]. It looks like the only viable alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pushing it with string[] for now. If you like it, we continue like that. Otherwise let me know and I change it to DeployTokenScope[] (or maybe a 3rd option?)

Copy link
Contributor

Choose a reason for hiding this comment

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

leaving it as string[] is probably our best bet

commitAction?: 'auto' | 'update' | 'delete' | 'create' | 'skip' | undefined
/**
* @public
* @deprecated use import from `@backstage/plugin-scaffolder-backend-module-github` instead
Copy link
Member

Choose a reason for hiding this comment

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

Wait where did this come from? :) I don't think I see it elsewhere in the code and this is an autogenerated file eh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it was generated automatically! 😕 It has nothing to do with my changes, that's why I kept it in a separate commit.. However, if I remember correctly, CI was checking that these api reports in my code match the generated ones, and this one didn't.
I can delete this, but I'm pretty sure it'll fail in CI.

How can I proceed?

Copy link
Member

Choose a reason for hiding this comment

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

Nah leave it in if that's how it got generated. Curious thing tho 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it wasn't added anymore by the api report generator.. weird 🤷

@freben
Copy link
Member

freben commented Nov 14, 2024

seems to need a rebase now too after the other PR got merged

Signed-off-by: mpsanchis <miguelpsanchis@gmail.com>
@mpsanchis mpsanchis force-pushed the mpsanchis/upgrade-gitbeaker-gitlab-scaffolder branch from ecef3cf to 08249ce Compare November 15, 2024 11:54
…eports

Signed-off-by: mpsanchis <miguelpsanchis@gmail.com>
@mpsanchis mpsanchis force-pushed the mpsanchis/upgrade-gitbeaker-gitlab-scaffolder branch from 08249ce to f84dba3 Compare November 17, 2024 13:41
@mpsanchis
Copy link
Contributor Author

@CptnFizzbin & @freben it seems that CI is green now 😄 Do we stay with the current state? Or do you want something else from my side?

.changeset/shy-teachers-count.md Outdated Show resolved Hide resolved
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
@freben freben added the merge-after-release This is a bit too scary to merge until after the next release label Nov 19, 2024
@freben freben merged commit db65221 into backstage:master Nov 21, 2024
29 checks passed
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:scaffolder Everything and all things related to the scaffolder project area merge-after-release This is a bit too scary to merge until after the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants