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

Adding PR submission section to contributors guide #43

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

pmccarthy
Copy link
Contributor

What
Adding new PR submission section to the contributors guide. This section is purposely high level as we'll most likely want to add some component specific guidelines in each repository

@openshift-ci openshift-ci bot requested review from eguzki and roivaz December 19, 2023 10:36
Copy link

@philbrookes philbrookes left a comment

Choose a reason for hiding this comment

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

This looks good to me, although it is not mandatory for a PR to have an associated issue referenced does appear to conflict with the PR workflow that's trying to be added to ensure every PR has a linked issue?

e.g. the one that's failing on this PR 😂


The Kuadrant project owners and maintainers strive to review and/or respond to all newly submitted PRs in a timely manner, however, if you're finding it difficult to get someone to review your PR, please reach out directly on [Slack][SlackChannelURL] or [mail][MailingList]

Finally, it is recommended that you squash your changes into a single commit where possible. If this is not feasible please ensure that your commits are representing a logical piece of work that can be reviewed independently within the PR.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @guicassolato I'll add something based on what you've referenced in the Developer guide. I think there's a much larger piece of work to be done here with respect to having a consistent message across the various Kuadrant repos calling back to a single developer guide with perhaps a few adjustments in each repo. I think the Authorino developer guide should form the basis for the others given it's depth and structure. I'll take an action to try progress this effort in the new year.

For now though I'll add the signed commits section and look to get this merged as a first pass

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Dec 19, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guicassolato, philbrookes, pmccarthy

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 be4b7b3 into Kuadrant:main Dec 19, 2023
1 of 2 checks passed
@pmccarthy
Copy link
Contributor Author

This looks good to me, although it is not mandatory for a PR to have an associated issue referenced does appear to conflict with the PR workflow that's trying to be added to ensure every PR has a linked issue?

e.g. the one that's failing on this PR 😂

Hmmm..I thought the purpose of that job was to ensure the PR and/or issue is showing on our project board? Rather than ensuring that there's an associated issue? Perhaps I'm wrong on that?

@guicassolato
Copy link
Contributor

Hmmm..I thought the purpose of that job was to ensure the PR and/or issue is showing on our project board? Rather than ensuring that there's an associated issue? Perhaps I'm wrong on that?

You are correct, @pmccarthy .

@philbrookes
Copy link

Hmmm..I thought the purpose of that job was to ensure the PR and/or issue is showing on our project board? Rather than ensuring that there's an associated issue? Perhaps I'm wrong on that?

You are correct, @pmccarthy .

oh, interesting, so if there is no linked issue, this job should not fail? I feel like it does currently.

@guicassolato
Copy link
Contributor

Hmmm..I thought the purpose of that job was to ensure the PR and/or issue is showing on our project board? Rather than ensuring that there's an associated issue? Perhaps I'm wrong on that?

You are correct, @pmccarthy .

oh, interesting, so if there is no linked issue, this job should not fail? I feel like it does currently.

We're still investigating why the job is failing, but I guess it has nothing to do with having or not having an issue linked to the PR. The PR itself is a kind of GitHub issue. AFAIK the job is only to set the Kuadrant project on all kinds of issues (actual issues or PRs.)

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.

3 participants