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

Clarify pull request usage in README #494

Conversation

tavaresrodrigo
Copy link
Contributor

@tavaresrodrigo tavaresrodrigo commented Jul 17, 2024

This pull request addresses an issue in the policy-collection README where the usage of pull requests may be confusing.

The current text suggests using a pull request to pull changes, which is not accurate. A pull request should be used to propose merging changes.

Changes Made:

  • Updated the sentence from:
    Fork this repository and use the forked version as the target to run the sync against. This is to
    avoid unintended changes to be applied to your cluster automatically. To get latest policies from
    the `policy-collection` repository, you can pull the latest changes from `policy-collection` to your
    own repository through a pull request. Any further changes to your repository are automatically be
    applied to your cluster.
    
    to:
    Fork this repository and use the forked version as the target to run the sync. This avoids unintended changes to your cluster. 
    To get the latest policies from the `policy-collection` repository, pull the latest changes from `policy-collection` and then create a pull request to merge these changes into your forked repository. Any further changes to your repository will be applied to your cluster automatically.
    

This change ensures that the instructions are clear and technically accurate, helping users understand the correct use of pull requests.

Signed-off-by: Rodrigo Tavares [email protected]

@dhaiducek
Copy link
Member

@tavaresrodrigo Thanks for the PR! For the DCO signoff, since there's only one commit you can run these commands to add the signoff:

git commit --amend --signoff --no-edit
git push --force

Copy link
Member

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

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

Thanks for the update! I think the wording looks good--just needs some linting and it'll be good to go!

README.md Outdated Show resolved Hide resolved
@dhaiducek
Copy link
Member

We prefer to minimize the number of commits--would you please squash these commits into a single commit? (Let me know if any guidance is needed on squashing.) After that, it should be good to merge.

Signed-off-by: Rodrigo Tavares <[email protected]>

Update README.md

Thanks @dhaiducek

Co-authored-by: Dale Haiducek <[email protected]>
Signed-off-by: Rodrigo Tavares <[email protected]>
@tavaresrodrigo tavaresrodrigo force-pushed the documentation-clarify-pr-usage branch from da5bb79 to 897d8cf Compare July 17, 2024 17:58
@tavaresrodrigo
Copy link
Contributor Author

Thanks for the helpful guidance @dhaiducek ! I have squashed all commits.

Copy link
Member

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @tavaresrodrigo!

@openshift-ci openshift-ci bot added the lgtm label Jul 18, 2024
Copy link

openshift-ci bot commented Jul 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, tavaresrodrigo

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 2b06406 into open-cluster-management-io:main Jul 18, 2024
3 checks passed
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.

2 participants