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.

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

fix(KONFLUX-5718): fix afterall to close merge request even it failed #1490

Merged
merged 1 commit into from
Jan 29, 2025
Merged

fix(KONFLUX-5718): fix afterall to close merge request even it failed #1490

merged 1 commit into from
Jan 29, 2025

Conversation

kasemAlem
Copy link
Contributor

@kasemAlem kasemAlem commented Jan 21, 2025 •
edited
Loading

On the Gitlab repo used to run the gitlab e2e-test suite , merge requests were not closed
as expected on the tear down of test , we fixed it by :

  • Close the opened MRs
  • Fix the AfterAll function so the MR will be closed even when test failed

Expected :
once we merge this fix we expect that no more old MRs opened

KONFLUX-5718

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added meaningful description with JIRA/GitHub issue key(if applicable), for example HASSuiteDescribe("STONE-123456789 devfile source")
  • I have updated labels (if needed)

hongweiliu17
hongweiliu17 approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@hongweiliu17 hongweiliu17 left a comment

Choose a reason for hiding this comment

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

lgtm

dirgim
dirgim approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 22, 2025
@kasemAlem
Copy link
Contributor Author

failed on ROSA provisioning known issue
/retest

@kasemAlem
Copy link
Contributor Author

/retest

@openshift-ci openshift-ci bot removed the lgtm label Jan 22, 2025
@kasemAlem
Copy link
Contributor Author

/retest

@hongweiliu17
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 23, 2025
dheerajodha
dheerajodha requested changes Jan 23, 2025
View reviewed changes
Copy link
Member

@dheerajodha dheerajodha left a comment

Choose a reason for hiding this comment

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

just so this doesn't get accidently merged before the open conversations are resolved

@openshift-ci openshift-ci bot removed the lgtm label Jan 24, 2025
@kasemAlem
Copy link
Contributor Author

kasemAlem commented Jan 24, 2025 •
edited
Loading

just so this doesn't get accidently merged before the open conversations are resolved

you are right, that was quite dumb solution , I've made the fix within the functions CloseMergeRequest and DeleteBranch , @dheerajodha wdyt?

@kasemAlem
Copy link
Contributor Author

/retest

@kasemAlem kasemAlem requested a review from dheerajodha January 26, 2025 04:46
dheerajodha
dheerajodha reviewed Jan 28, 2025
View reviewed changes
dheerajodha
dheerajodha approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member

@dheerajodha dheerajodha left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 29, 2025
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jan 29, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dheerajodha, dirgim, hongweiliu17, jsztuka

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 118ccac into konflux-ci:main Jan 29, 2025
13 checks passed
This was referenced Jan 29, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dheerajodha dheerajodha dheerajodha approved these changes

@dirgim dirgim dirgim approved these changes

@hongweiliu17 hongweiliu17 hongweiliu17 approved these changes

@jsztuka jsztuka jsztuka approved these changes

@14rcole 14rcole Awaiting requested review from 14rcole

@tisutisu tisutisu Awaiting requested review from tisutisu

@Josh-Everett Josh-Everett Awaiting requested review from Josh-Everett

@jencull jencull Awaiting requested review from jencull

Assignees

@dirgim dirgim

@hongweiliu17 hongweiliu17

@dheerajodha dheerajodha

Labels
approved lgtm < 1 min
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kasemAlem @hongweiliu17 @dirgim @dheerajodha @jsztuka