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

[stable] Fix naming of the helper.go file so it follows the golang conventions #379

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

jstourac
Copy link
Member

@jstourac jstourac commented Aug 13, 2024

https://issues.redhat.com/browse/RHOAIENG-11259

This name caused go vulnerability check to fail since this file is part
of the tests and relevant other test files weren't compiled because they
have their _test inffix as expected.

Note: this is also alignment to what is in v1.7-branch already.

Description

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@jstourac
Copy link
Member Author

Okay, so we can see all is green now. I'm gonna remove the CVE fix commit from this PR now, which will be introduced by the #378 anyway.

@jstourac jstourac requested a review from jiridanek August 13, 2024 08:00
@jstourac jstourac changed the title Fix naming Fix naming of the helper.go file so it follows the golang conventions Aug 13, 2024
@jiridanek
Copy link
Member

/lgtm

@caponetto
Copy link

/lgtm

@jstourac
Copy link
Member Author

/approve

@jstourac jstourac changed the title Fix naming of the helper.go file so it follows the golang conventions [stable] Fix naming of the helper.go file so it follows the golang conventions Aug 13, 2024
@harshad16
Copy link
Member

Thanks for fix
/approve

@harshad16
Copy link
Member

harshad16 commented Aug 13, 2024

Error seems to be for the x/net , which are fixed in #378
a rebase should fix this issue.

https://issues.redhat.com/browse/RHOAIENG-11259

This name caused go vulnerability check to fail since this file is part
of the tests and relevant other test files weren't compiled because they
have their _test inffix as expected.

Note: this is also alignment to what is in v1.7-branch already.
@harshad16
Copy link
Member

Thank you for rebasing
/lgtm
/approve

@jstourac
Copy link
Member Author

jstourac commented Aug 13, 2024

/approve 😄

I guess that another approve via GH UI needs to be done.

@jiridanek
Copy link
Member

jiridanek commented Aug 13, 2024

I guess that another approve via GH UI needs to be done.

By harshad, andriana, or adriel, our approves don't count there

Merging can be performed automatically with 2 approving reviews.

@harshad16
Copy link
Member

By harshad, andriana, or adriel, our approves don't count there

I think @jiridanek , prow is considering approver from all the people in approvers list.
see this:
Screenshot from 2024-08-13 09-46-57

@jstourac
Copy link
Member Author

jstourac commented Aug 13, 2024

By harshad, andriana, or adriel, our approves don't count there

I think @jiridanek , prow is considering approver from all the people in approvers list. see this: Screenshot from 2024-08-13 09-46-57

Hm, but here it looks like it truly waits for the UI approve which needs to be done by the people set in the repo settings, not in the owners?

Merging can be performed automatically with 2 approving reviews.

@harshad16
Copy link
Member

but here it looks like it truly waits for the UI approve which needs to be done by the people set in the repo settings, not in the owners?

@jstourac , i didn't get that completely
can you explain me a little more?

@jstourac jstourac requested a review from atheo89 August 13, 2024 14:50
@jstourac
Copy link
Member Author

but here it looks like it truly waits for the UI approve which needs to be done by the people set in the repo settings, not in the owners?

@jstourac , i didn't get that completely can you explain me a little more?

I mean, it looks that the settings of this repository waits first to have two approvers that are done via GitHub UI, not via /approve command. Only then this PR is marked as possible to merge. And this approve must be done by only a couple of people that are set in the repository settings, irrelevant to values in the OWNERS file in the repository. Which is basically people that Jiri mentioned above.

I'm not expert in this, I'm usually lost in our repository settings WRT merging bots, so I can be wrong. But as you can see, once you approved, there is 1 approval shown at the end of this page. I suppose once there are 2, this can be moved further. 🙂

@atheo89
Copy link
Member

atheo89 commented Aug 14, 2024

Thanks Jan!

/lgtm
/approve

Copy link
Member

@atheo89 atheo89 left a comment

Choose a reason for hiding this comment

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

Let's see if the ui approve make difference!

Copy link

openshift-ci bot commented Aug 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atheo89, harshad16, jiridanek, jstourac

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:
  • OWNERS [atheo89,harshad16,jiridanek,jstourac]

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

@jstourac
Copy link
Member Author

Pending — Not mergeable. No Tide query for branch stable found.

Looks like tide bot won't work here on this branch...

@jiridanek
Copy link
Member

jiridanek commented Aug 14, 2024

But we met the GitHub merge criteria, now anyone with write access to the repository can merge this, as the last line says

@jiridanek
Copy link
Member

image

@jstourac
Copy link
Member Author

So openshift/release#55570 is merged. Let's see whether this will be grabbed automatically somehow now... 🫤

@openshift-merge-bot openshift-merge-bot bot merged commit 7d265db into opendatahub-io:stable Aug 14, 2024
7 checks passed
@jstourac jstourac deleted the fixNaming branch August 14, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants