Skip to content

feat(api): add merge request pipeline manager and deprecate mr.pipelines() method #1323

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 14, 2021
Merged

feat(api): add merge request pipeline manager and deprecate mr.pipelines() method #1323

merged 1 commit into from
Jun 14, 2021

Conversation

nejch
Copy link
Member

@nejch nejch commented Feb 23, 2021 •
edited
Loading

Closes #1312.
Closes #1239.

todo: technically this breaks the CLI gitlab project-merge-request pipelines and requires project-merge-request-pipelines list instead. I'll see if I can hack the register_custom_action decorator to accept custom action names, not just the current method name. I had to remove it as there would be a namespace clash between the old method and the new manager.

Well, I've managed to hack register_custom_action a little so it takes an optional custom_action argument to override the current function name as the CLI action, so should be backwards compatible, but then realized the old method probably never worked in CLI due to missing CLI args 🤦

Still, now you can do (until 3.0.0 or so, but probably useless anyway):

gitlab project-merge-request pipelines --help
usage: gitlab project-merge-request pipelines [-h] --project-id PROJECT_ID [--sudo SUDO] --iid IID

optional arguments:
  -h, --help            show this help message and exit
  --project-id PROJECT_ID
  --sudo SUDO
  --iid IID

And the new list, with proper help and args:

gitlab project-merge-request-pipeline list --help
usage: gitlab merge-request-pipeline list [-h] [--sudo SUDO] --project-id PROJECT_ID --mr-iid MR_IID
                                          [--page PAGE] [--per-page PER_PAGE] [--all]

optional arguments:
  -h, --help            show this help message and exit
  --sudo SUDO
  --project-id PROJECT_ID
  --mr-iid MR_IID
  --page PAGE
  --per-page PER_PAGE
  --all

@nejch nejch mentioned this pull request Feb 23, 2021
Closed
@nejch nejch force-pushed the feat/mr-pipeline-manager branch 3 times, most recently from c8fed5a to f12841f Compare February 26, 2021 19:35
@nejch nejch marked this pull request as ready for review February 26, 2021 19:49
@nejch
Copy link
Member Author

nejch commented Feb 26, 2021

Maybe @JohnVillalovos you want to have a look at the typing here as well, I always forget decorators and have to google the wrap functions every time I work with them 🤣

@JohnVillalovos
Copy link
Member

Maybe @JohnVillalovos you want to have a look at the typing here as well, I always forget decorators and have to google the wrap functions every time I work with them 🤣

From my initial look it seems correct. Also mypy is checking that gitlab/cli.py file, so seems good.

Looks Good To Me (LGTM) 👍

Thanks!

@max-wittig
Copy link
Member

We're most likely gonna go to 3.0.0 directly, because of this breaking change: #1278. Maybe have those two in one release would be nice 😄

@msune
Copy link

msune commented Mar 29, 2021

👍 on this.

I seem to be hitting a limitation/bug of pipelines() #1312 (comment) and in another issue that I will not bother mentioning.

Any chance this could be integrated soon?

@nejch nejch added this to the v3.0.0 milestone Apr 27, 2021
@max-wittig
Copy link
Member

@nejch Sorry for making you wait for long that everything conflicts. I just see like 15 notifications everyday from this project. You people are crazy 😄

Would you mind to rebase?

@max-wittig max-wittig assigned nejch and unassigned max-wittig May 31, 2021
@nejch nejch force-pushed the feat/mr-pipeline-manager branch from f12841f to abe4242 Compare June 13, 2021 18:00
JohnVillalovos
JohnVillalovos requested changes Jun 13, 2021
View reviewed changes
Copy link
Member

@JohnVillalovos JohnVillalovos left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Left one thing I think should be fixed.

@nejch nejch force-pushed the feat/mr-pipeline-manager branch from abe4242 to 954357c Compare June 13, 2021 19:19
JohnVillalovos
JohnVillalovos approved these changes Jun 13, 2021
View reviewed changes
Copy link
Member

@JohnVillalovos JohnVillalovos left a comment

Choose a reason for hiding this comment

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

Thanks. This looks good.

I'll let you merge it or if you want to wait for @max-wittig to review. Your choice.

@JohnVillalovos JohnVillalovos merged commit e77554c into master Jun 14, 2021
@JohnVillalovos JohnVillalovos deleted the feat/mr-pipeline-manager branch June 14, 2021 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnVillalovos JohnVillalovos JohnVillalovos approved these changes

Assignees

@nejch nejch

Labels
None yet
Projects
None yet
Milestone
v3.0.0
4 participants
@nejch @JohnVillalovos @max-wittig @msune